On Wed, Jan 04, 2017 at 12:11:20AM +0000, Bryan O'Donoghue wrote:
Loopback has its own internal method for tracking and timing out asynchronous operations however previous patches make it possible to use functionality provided by operation.c to do this instead. Using the code in operation.c means we can completely subtract the timer, the work-queue, the kref and the cringe-worthy 'pending' flag. The completion callback triggered by operation.c will provide an authoritative result code - including -ETIMEDOUT for asynchronous operations. Tested to ensure the existing stats given by loopback for synchronous, asynchronous and asynchronous+timeout still hold.
Good that you verified and mention that here, but not sure adding all the numbers below is needed. What test setup are you using by the way?
Also note that this one no longer applies due to a trivial clean up patch in staging-next.
A couple of comments below.
Before: gb_loopback_test -i 1000 -s 128 -t sink -p 1970-1-1 7:30:8 test: sink path: gb_loopback0 size: 128 iterations: 1000 errors: 0 async: Disabled requests per-sec: min=432, max=436, average=434.662994, jitter=4 ap-throughput B/s: min=67535 max=68036 average=67807.421875 jitter=501 ap-latency usec: min=1626 max=3559 average=2287.077881 jitter=1933 apbridge-latency usec: min=0 max=0 average=0.000000 jitter=0 gbphy-latency usec: min=0 max=0 average=0.000000 jitter=0
[...]
Signed-off-by: Bryan O'Donoghue pure.logic@nexus-software.ie Signed-off-by: Mitchell Tasman tasman@leaflabs.com
drivers/staging/greybus/loopback.c | 167 +++++++------------------------------ 1 file changed, 32 insertions(+), 135 deletions(-)
@@ -615,32 +513,29 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type, if (request_size) memcpy(operation->request->payload, request, request_size);
- gb_operation_set_data(operation, op_async);
- op_async->gb = gb; op_async->operation = operation; op_async->completion = completion;
- spin_lock_irqsave(&gb_dev.lock, flags);
- list_add_tail(&op_async->entry, &gb_dev.list_op_async);
- spin_unlock_irqrestore(&gb_dev.lock, flags);
- do_gettimeofday(&op_async->ts);
- op_async->pending = true;
- atomic_inc(&gb->outstanding_operations);
- mutex_lock(&gb->mutex);
Do you still need this lock?
- ret = gb_operation_request_send(operation,
- ret = gb_operation_request_send_timeout(operation,
jiffies_to_msecs(gb->jiffy_timeout), gb_loopback_async_operation_callback, GFP_KERNEL);
- setup_timer(&op_async->timer, gb_loopback_async_operation_timeout,
(unsigned long)operation->id);- op_async->timer.expires = jiffies + gb->jiffy_timeout;
- add_timer(&op_async->timer);
- goto done;
error:
- gb_loopback_async_operation_put(op_async);
- atomic_dec(&gb->outstanding_operations);
- gb_operation_put(operation);
- kfree(op_async);
done: mutex_unlock(&gb->mutex); return ret; @@ -1045,8 +940,10 @@ static int gb_loopback_fn(void *data) error = gb_loopback_async_sink(gb, size); }
if (error)
if (error) { gb->error++;gb->iteration_count++;}
This is an unrelated bug fix that should go in it's own patch, right?
The iteration count should be incremented on errors regardless of this change.
} else { /* We are effectively single threaded here */ if (type == GB_LOOPBACK_TYPE_PING)
Thanks, Johan