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@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Mitchell Tasman <tasman@xxxxxxxxxxxx>
> ---
> 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);
> if (ret)
> goto error;
>
> - 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
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev
|