Make the devlock a cglock in usbutils and only grab the write lock for fundamental changes allowing us to send and receive transfers concurrently without lock contention.

cgminer.c

@@ -58,6 +58,9 @@ char *curly = ":D";
 #include "driver-opencl.h"
 #include "bench_block.h"
 #include "scrypt.h"
+#ifdef USE_USBUTILS
+#include "usbutils.h"
+#endif
 
 #ifdef USE_AVALON
 #include "driver-avalon.h"

driver-modminer.c

@@ -206,7 +206,7 @@ static bool modminer_detect_one(struct libusb_device *dev, struct usb_find_devic
 		tmp->modminer_mutex = modminer->modminer_mutex;
 
 		if (!add_cgpu(tmp)) {
-			tmp = usb_free_cgpu_devlock(tmp, !added);
+			tmp = usb_free_cgpu(tmp);
 			goto unshin;
 		}
 
@@ -215,7 +215,7 @@ static bool modminer_detect_one(struct libusb_device *dev, struct usb_find_devic
 		added = true;
 	}
 
-	modminer = usb_free_cgpu_devlock(modminer, !added);
+	modminer = usb_free_cgpu(modminer);
 
 	return true;
 
@@ -229,7 +229,7 @@ shin:
 		modminer->modminer_mutex = NULL;
 	}
 
-	modminer = usb_free_cgpu_devlock(modminer, !added);
+	modminer = usb_free_cgpu(modminer);
 
 	if (added)
 		return true;

miner.h

@@ -821,14 +821,6 @@ static inline void _rwlock_init(pthread_rwlock_t *lock, const char *file, const
 		quitfrom(1, file, func, line, "Failed to pthread_rwlock_init errno=%d", errno);
 }
 
-/* cgminer locks, a write biased variant of rwlocks */
-struct cglock {
-	pthread_mutex_t mutex;
-	pthread_rwlock_t rwlock;
-};
-
-typedef struct cglock cglock_t;
-
 static inline void cglock_init(cglock_t *lock)
 {
 	mutex_init(&lock->mutex);

usbutils.c

@@ -32,13 +32,23 @@
  *  You must call DEVUNLOCK(cgpu, pstate) before exiting the function or it will leave
  *  the thread Cancelability unrestored
  */
-#define DEVLOCK(cgpu, _pth_state) do { \
+#define DEVWLOCK(cgpu, _pth_state) do { \
 			pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &_pth_state); \
-			mutex_lock(cgpu->usbinfo.devlock); \
+			cg_wlock(&cgpu->usbinfo.devlock); \
 			} while (0)
 
-#define DEVUNLOCK(cgpu, _pth_state) do { \
-			mutex_unlock(cgpu->usbinfo.devlock); \
+#define DEVWUNLOCK(cgpu, _pth_state) do { \
+			cg_wunlock(&cgpu->usbinfo.devlock); \
+			pthread_setcancelstate(_pth_state, NULL); \
+			} while (0)
+
+#define DEVRLOCK(cgpu, _pth_state) do { \
+			pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &_pth_state); \
+			cg_rlock(&cgpu->usbinfo.devlock); \
+			} while (0)
+
+#define DEVRUNLOCK(cgpu, _pth_state) do { \
+			cg_runlock(&cgpu->usbinfo.devlock); \
 			pthread_setcancelstate(_pth_state, NULL); \
 			} while (0)
 
@@ -1268,14 +1278,14 @@ static void _usb_uninit(struct cgpu_info *cgpu)
 {
 	int ifinfo;
 
-	applog(LOG_DEBUG, "USB uninit %s%i",
-			cgpu->drv->name, cgpu->device_id);
-
 	// May have happened already during a failed initialisation
 	//  if release_cgpu() was called due to a USB NODEV(err)
 	if (!cgpu->usbdev)
 		return;
 
+	applog(LOG_DEBUG, "USB uninit %s%i",
+			cgpu->drv->name, cgpu->device_id);
+
 	if (cgpu->usbdev->handle) {
 		for (ifinfo = cgpu->usbdev->found->intinfo_count - 1; ifinfo >= 0; ifinfo--) {
 			libusb_release_interface(cgpu->usbdev->handle,
@@ -1296,29 +1306,29 @@ void usb_uninit(struct cgpu_info *cgpu)
 {
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	_usb_uninit(cgpu);
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
-/*
- * N.B. this is always called inside
- *	DEVLOCK(cgpu, pstate);
- */
+/* We have dropped the read devlock before entering this function but we pick
+ * up the write lock to prevent any attempts to work on dereferenced code once
+ * the nodev flag has been set. */
 static void release_cgpu(struct cgpu_info *cgpu)
 {
 	struct cg_usb_device *cgusb = cgpu->usbdev;
 	struct cgpu_info *lookcgpu;
-	int i;
-
-	applog(LOG_DEBUG, "USB release %s%i",
-			cgpu->drv->name, cgpu->device_id);
+	int i, pstate;
 
+	DEVWLOCK(cgpu, pstate);
 	// It has already been done
 	if (cgpu->usbinfo.nodev)
-		return;
+		goto out_unlock;
+
+	applog(LOG_DEBUG, "USB release %s%i",
+			cgpu->drv->name, cgpu->device_id);
 
 	zombie_devs++;
 	total_count--;
@@ -1345,6 +1355,8 @@ static void release_cgpu(struct cgpu_info *cgpu)
 
 	_usb_uninit(cgpu);
 	cgminer_usb_unlock_bd(cgpu->drv, cgpu->usbinfo.bus_number, cgpu->usbinfo.device_address);
+out_unlock:
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 /*
@@ -1355,7 +1367,7 @@ struct cgpu_info *usb_copy_cgpu(struct cgpu_info *orig)
 	struct cgpu_info *copy;
 	int pstate;
 
-	DEVLOCK(orig, pstate);
+	DEVWLOCK(orig, pstate);
 
 	copy = calloc(1, sizeof(*copy));
 	if (unlikely(!copy))
@@ -1372,7 +1384,7 @@ struct cgpu_info *usb_copy_cgpu(struct cgpu_info *orig)
 
 	copy->usbinfo.nodev = (copy->usbdev == NULL);
 
-	DEVUNLOCK(orig, pstate);
+	DEVWUNLOCK(orig, pstate);
 
 	return copy;
 }
@@ -1390,25 +1402,18 @@ struct cgpu_info *usb_alloc_cgpu(struct device_drv *drv, int threads)
 
 	cgpu->usbinfo.nodev = true;
 
-	cgpu->usbinfo.devlock = calloc(1, sizeof(*(cgpu->usbinfo.devlock)));
-	if (unlikely(!cgpu->usbinfo.devlock))
-		quit(1, "Failed to calloc devlock for %s in usb_alloc_cgpu", drv->dname);
-
-	mutex_init(cgpu->usbinfo.devlock);
+	cglock_init(&cgpu->usbinfo.devlock);
 
 	return cgpu;
 }
 
-struct cgpu_info *usb_free_cgpu_devlock(struct cgpu_info *cgpu, bool free_devlock)
+struct cgpu_info *usb_free_cgpu(struct cgpu_info *cgpu)
 {
 	if (cgpu->drv->copy)
 		free(cgpu->drv);
 
 	free(cgpu->device_path);
 
-	if (free_devlock)
-		free(cgpu->usbinfo.devlock);
-
 	free(cgpu);
 
 	return NULL;
@@ -1431,7 +1436,7 @@ static int _usb_init(struct cgpu_info *cgpu, struct libusb_device *dev, struct u
 	int bad = USB_INIT_FAIL;
 	int cfg, claimed = 0;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	cgpu->usbinfo.bus_number = libusb_get_bus_number(dev);
 	cgpu->usbinfo.device_address = libusb_get_device_address(dev);
@@ -1748,7 +1753,7 @@ dame:
 	cgusb = free_cgusb(cgusb);
 
 out_unlock:
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 
 	return bad;
 }
@@ -2360,7 +2365,7 @@ int _usb_read(struct cgpu_info *cgpu, int intinfo, int epinfo, char *buf, size_t
 	unsigned char *ptr, *usbbuf = cgpu->usbinfo.bulkbuf;
 	size_t usbbufread;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	if (cgpu->usbinfo.nodev) {
 		*buf = '\0';
@@ -2480,9 +2485,6 @@ int _usb_read(struct cgpu_info *cgpu, int intinfo, int epinfo, char *buf, size_t
 		*processed = tot;
 		memcpy((char *)buf, (const char *)usbbuf, (tot < (int)bufsiz) ? tot + 1 : (int)bufsiz);
 
-		if (NODEV(err))
-			release_cgpu(cgpu);
-
 		goto out_unlock;
 	}
 
@@ -2608,16 +2610,16 @@ int _usb_read(struct cgpu_info *cgpu, int intinfo, int epinfo, char *buf, size_t
 	*processed = tot;
 	memcpy((char *)buf, (const char *)usbbuf, (tot < (int)bufsiz) ? tot + 1 : (int)bufsiz);
 
-	if (NODEV(err))
-		release_cgpu(cgpu);
-
 out_unlock:
 	if (err && err != LIBUSB_ERROR_TIMEOUT && err != LIBUSB_TRANSFER_TIMED_OUT) {
 		applog(LOG_WARNING, "%s %i usb read error: %s", cgpu->drv->name, cgpu->device_id,
 		       libusb_error_name(err));
 	}
 out_noerrmsg:
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
+
+	if (NODEV(err))
+		release_cgpu(cgpu);
 
 	return err;
 }
@@ -2631,7 +2633,7 @@ int _usb_write(struct cgpu_info *cgpu, int intinfo, int epinfo, char *buf, size_
 	__maybe_unused bool first = true;
 	int err, sent, tot, pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	USBDEBUG("USB debug: _usb_write(%s (nodev=%s),intinfo=%d,epinfo=%d,buf='%s',bufsiz=%d,proc=%p,timeout=%u,cmd=%s)", cgpu->drv->name, bool_str(cgpu->usbinfo.nodev), intinfo, epinfo, (char *)str_text(buf), (int)bufsiz, processed, timeout, usb_cmdname(cmd));
 
@@ -2705,15 +2707,15 @@ int _usb_write(struct cgpu_info *cgpu, int intinfo, int epinfo, char *buf, size_
 
 	*processed = tot;
 
-	if (NODEV(err))
-		release_cgpu(cgpu);
-
 	if (err) {
 		applog(LOG_WARNING, "%s %i usb write error: %s", cgpu->drv->name, cgpu->device_id,
 		       libusb_error_name(err));
 	}
 out_noerrmsg:
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
+
+	if (NODEV(err))
+		release_cgpu(cgpu);
 
 	return err;
 }
@@ -2784,9 +2786,6 @@ int __usb_transfer(struct cgpu_info *cgpu, uint8_t request_type, uint8_t bReques
 
 	IOERR_CHECK(cgpu, err);
 
-	if (NOCONTROLDEV(err))
-		release_cgpu(cgpu);
-
 	if (err < 0 && err != LIBUSB_ERROR_TIMEOUT) {
 		applog(LOG_WARNING, "%s %i usb transfer error: %s", cgpu->drv->name, cgpu->device_id,
 		       libusb_error_name(err));
@@ -2799,11 +2798,14 @@ int _usb_transfer(struct cgpu_info *cgpu, uint8_t request_type, uint8_t bRequest
 {
 	int pstate, err;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	err = __usb_transfer(cgpu, request_type, bRequest, wValue, wIndex, data, siz, timeout, cmd);
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
+
+	if (NOCONTROLDEV(err))
+		release_cgpu(cgpu);
 
 	return err;
 }
@@ -2817,7 +2819,7 @@ int _usb_transfer_read(struct cgpu_info *cgpu, uint8_t request_type, uint8_t bRe
 	unsigned char tbuf[64];
 	int err, pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	USBDEBUG("USB debug: _usb_transfer_read(%s (nodev=%s),type=%"PRIu8",req=%"PRIu8",value=%"PRIu16",index=%"PRIu16",bufsiz=%d,timeout=%u,cmd=%s)", cgpu->drv->name, bool_str(cgpu->usbinfo.nodev), request_type, bRequest, wValue, wIndex, bufsiz, timeout, usb_cmdname(cmd));
 
@@ -2867,15 +2869,16 @@ int _usb_transfer_read(struct cgpu_info *cgpu, uint8_t request_type, uint8_t bRe
 	if (err > 0) {
 		*amount = err;
 		err = 0;
-	} else if (NOCONTROLDEV(err))
-		release_cgpu(cgpu);
-
+	}
 	if (err < 0 && err != LIBUSB_ERROR_TIMEOUT) {
 		applog(LOG_WARNING, "%s %i usb transfer read error: %s", cgpu->drv->name, cgpu->device_id,
 		       libusb_error_name(err));
 	}
 out_noerrmsg:
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
+
+	if (NOCONTROLDEV(err))
+		release_cgpu(cgpu);
 
 	return err;
 }
@@ -2909,7 +2912,7 @@ int _usb_ftdi_set_latency(struct cgpu_info *cgpu, int intinfo)
 	int err = 0;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev) {
 		if (cgpu->usbdev->usb_type != USB_TYPE_FTDI) {
@@ -2929,7 +2932,7 @@ int _usb_ftdi_set_latency(struct cgpu_info *cgpu, int intinfo)
 						NULL, 0, DEVTIMEOUT, C_LATENCY);
 	}
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 
 	applog(LOG_DEBUG, "%s: cgid %d %s got err %d",
 				cgpu->drv->name, cgpu->cgminer_id,
@@ -2943,7 +2946,7 @@ void usb_buffer_enable(struct cgpu_info *cgpu)
 	struct cg_usb_device *cgusb;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	cgusb = cgpu->usbdev;
 	if (cgusb && !cgusb->buffer) {
@@ -2955,7 +2958,7 @@ void usb_buffer_enable(struct cgpu_info *cgpu)
 		cgusb->bufsiz = USB_MAX_READ;
 	}
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 void usb_buffer_disable(struct cgpu_info *cgpu)
@@ -2963,7 +2966,7 @@ void usb_buffer_disable(struct cgpu_info *cgpu)
 	struct cg_usb_device *cgusb;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	cgusb = cgpu->usbdev;
 	if (cgusb && cgusb->buffer) {
@@ -2973,19 +2976,19 @@ void usb_buffer_disable(struct cgpu_info *cgpu)
 		cgusb->buffer = NULL;
 	}
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 void usb_buffer_clear(struct cgpu_info *cgpu)
 {
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		cgpu->usbdev->bufamt = 0;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 uint32_t usb_buffer_size(struct cgpu_info *cgpu)
@@ -2993,12 +2996,12 @@ uint32_t usb_buffer_size(struct cgpu_info *cgpu)
 	uint32_t ret = 0;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		ret = cgpu->usbdev->bufamt;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
 
 	return ret;
 }
@@ -3007,36 +3010,36 @@ void usb_set_cps(struct cgpu_info *cgpu, int cps)
 {
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		cgpu->usbdev->cps = cps;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 void usb_enable_cps(struct cgpu_info *cgpu)
 {
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		cgpu->usbdev->usecps = true;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 void usb_disable_cps(struct cgpu_info *cgpu)
 {
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		cgpu->usbdev->usecps = false;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 /*
@@ -3051,12 +3054,12 @@ int _usb_interface(struct cgpu_info *cgpu, int intinfo)
 	int interface = 0;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		interface = cgpu->usbdev->found->intinfos[intinfo].ctrl_transfer;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
 
 	return interface;
 }
@@ -3066,12 +3069,12 @@ enum sub_ident usb_ident(struct cgpu_info *cgpu)
 	enum sub_ident ident = IDENT_UNK;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVRLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev)
 		ident = cgpu->usbdev->ident;
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVRUNLOCK(cgpu, pstate);
 
 	return ident;
 }
@@ -3089,7 +3092,7 @@ void _usb_set_pps(struct cgpu_info *cgpu, int intinfo, int epinfo, uint16_t Pref
 	struct usb_find_devices *found;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	if (cgpu->usbdev) {
 		found = cgpu->usbdev->found;
@@ -3107,7 +3110,7 @@ void _usb_set_pps(struct cgpu_info *cgpu, int intinfo, int epinfo, uint16_t Pref
 		}
 	}
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
 // Need to set all devices with matching usbdev
@@ -3118,7 +3121,7 @@ void usb_set_dev_start(struct cgpu_info *cgpu)
 	struct timeval now;
 	int pstate;
 
-	DEVLOCK(cgpu, pstate);
+	DEVWLOCK(cgpu, pstate);
 
 	cgusb = cgpu->usbdev;
 
@@ -3135,10 +3138,10 @@ void usb_set_dev_start(struct cgpu_info *cgpu)
 		}
 	}
 
-	DEVUNLOCK(cgpu, pstate);
+	DEVWUNLOCK(cgpu, pstate);
 }
 
-void usb_cleanup()
+void usb_cleanup(void)
 {
 	struct cgpu_info *cgpu;
 	int count;
@@ -3159,9 +3162,7 @@ void usb_cleanup()
 			case DRIVER_icarus:
 			case DRIVER_avalon:
 			case DRIVER_klondike:
-				mutex_lock(cgpu->usbinfo.devlock);
 				release_cgpu(cgpu);
-				mutex_unlock(cgpu->usbinfo.devlock);
 				count++;
 				break;
 			default:

usbutils.h

@@ -242,7 +242,7 @@ struct cg_usb_info {
 	 * that uses the lock - however, all usbutils code MUST use it
 	 * to avoid devices disappearing while in use by multiple threads
 	 */
-	pthread_mutex_t *devlock;
+	cglock_t devlock;
 
 	time_t last_pipe;
 	uint64_t pipe_count;
@@ -362,8 +362,7 @@ const char *usb_cmdname(enum usb_cmds cmd);
 void usb_applog(struct cgpu_info *bflsc, enum usb_cmds cmd, char *msg, int amount, int err);
 struct cgpu_info *usb_copy_cgpu(struct cgpu_info *orig);
 struct cgpu_info *usb_alloc_cgpu(struct device_drv *drv, int threads);
-struct cgpu_info *usb_free_cgpu_devlock(struct cgpu_info *cgpu, bool free_devlock);
-#define usb_free_cgpu(cgpu) usb_free_cgpu_devlock(cgpu, true)
+struct cgpu_info *usb_free_cgpu(struct cgpu_info *cgpu);
 void usb_uninit(struct cgpu_info *cgpu);
 bool usb_init(struct cgpu_info *cgpu, struct libusb_device *dev, struct usb_find_devices *found);
 void usb_detect(struct device_drv *drv, bool (*device_detect)(struct libusb_device *, struct usb_find_devices *));

util.h

@@ -58,6 +58,14 @@ typedef curl_proxytype proxytypes_t;
 typedef int proxytypes_t;
 #endif /* HAVE_LIBCURL */
 
+/* cgminer locks, a write biased variant of rwlocks */
+struct cglock {
+	pthread_mutex_t mutex;
+	pthread_rwlock_t rwlock;
+};
+
+typedef struct cglock cglock_t;
+
 /* cgminer specific unnamed semaphore implementations to cope with osx not
  * implementing them. */
 #ifdef __APPLE__