netdev - Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 02 Sep 2014 12:27:19 -0400
From:	David L Stevens <david.stevens@...cle.com>
To:	Sowmini Varadhan <sowmini.varadhan@...cle.com>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor
 after short udelay()

Sowmini,
	One of the things I found while looking at the code is that the
write memory barrier is in the wrong place. I have a patch to fix it, but
it means the last descriptor will NOT be marked as ready until the cache
is flushed, which may be delayed.
	That fix may make this one moot. Maybe not, but certainly there is
an unnecessary delay in notifying the peer because of that bug. The fix is
simply to move the "wmb()" after, instead of before, setting VIO_DESC_READY.
I mentioned it in our stand-up last week, which you couldn't attend, because
I pointed it out for the VDC driver, which also has the same problem.

	You may want to retry with that change to see if the delay still
helps.
							+-DLS

On 09/02/2014 12:20 PM, Sowmini Varadhan wrote:
> 
> Upon encountering the first !VIO_DESC_READY in vnet_walk_rx(),
> it is frequently worthwhile to re-check the descriptor status
> after a short microsecond delay, as a bursty sender could
> be actively populating the descriptors, and the short udelay()
> is far less expensive than rolling back to ldc_rx() and having
> to wake up and read data on another LDC message.
> 
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
> Acked-by: Raghuram Kothakota <raghuram.kothakota@...cle.com>
> ---
> changes since v1: moved label `again', variable `retries' to this patchset.
> 
>  drivers/net/ethernet/sun/sunvnet.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
> index 8f90b57..7b1f320 100644
> --- a/drivers/net/ethernet/sun/sunvnet.c
> +++ b/drivers/net/ethernet/sun/sunvnet.c
> @@ -390,11 +390,28 @@ static int vnet_walk_rx(struct vnet_port *port, struct vio_dring_state *dr,
>  	viodbg(DATA, "vnet_walk_rx start[%08x] end[%08x]\n", start, end);
>  
>  	while (start != end) {
> -		int ack = 0, err = vnet_walk_rx_one(port, dr, start, &ack);
> +		int retries;
> +		int ack = 0, err;
> +
> +		retries = 0;
> +again:
> +		err = vnet_walk_rx_one(port, dr, start, &ack);
>  		if (err == -ECONNRESET)
>  			return err;
> -		if (err != 0)
> -			break;
> +		if (err != 0)  {
> +			/* The descriptor was not READY. Retry with a
> +			 * small delay, in case we have a bursty sender
> +			 * that is actively populating the descriptors, to
> +			 * reduce the overhead of stopping and re-entering
> +			 * which would involve expensive LDC messages.
> +			 */
> +			if (retries++ < 3) {
> +				udelay(4);
> +				goto again;
> +			} else {
> +				break;
> +			}
> +		}
>  		if (ack_start == -1)
>  			ack_start = start;
>  		ack_end = start;
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html