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>] [day] [month] [year] [list]

Date:	Sun, 7 Sep 2014 12:36:11 -0700
From:	Raghuram Kothakota <Raghuram.Kothakota@...cle.com>
To:	Sowmini Varadhan <sowmini.varadhan@...cle.com>
Cc:	David Miller <davem@...emloft.net>, david.stevens@...cle.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data descriptor after short udelay()

Sorry for joining this thread late, please see my response below:

On Sep 7, 2014, at 11:15 AM, Sowmini Varadhan <sowmini.varadhan@...cle.com> wrote:

> 
> 
> On (09/06/14 17:02), Sowmini Varadhan wrote:
>> Date: Sat, 6 Sep 2014 17:02:53 -0400
>> From: Sowmini Varadhan <sowmini.varadhan@...cle.com>
>> To: David Miller <davem@...emloft.net>
>> Cc: david.stevens@...cle.com, netdev@...r.kernel.org
>> Subject: Re: [PATCH net-next v2 2/2] Re-check for a VIO_DESC_READY data
>> descriptor after short udelay()
>> User-Agent: Mutt/1.5.21 (2010-09-15)
>> 
>> On (09/05/14 09:47), Sowmini Varadhan wrote:
>>>> The memory barrier exists in order to make sure the cookies et al. are
>>>> globally visible before the VIO_DESC_READY.  We don't want stores to
>>>> be reordered such that the VIO_DESC_READY is seen too early.
>>> 
>>> Ok, though David (dls) was just pointing out that a rmb() might
>>> be missing in vnet_walk_rx_one() before checking for READY descriptor
>> 
>> Stared at this a bit over the last two days, checked
>> the documentation, discussed with dls offline - looks like 
>> (a) the rmb() thing was mostly a red-herring/fud 
>> (b) we do need the wmb()
>> 
>> The wmb() part is working correctly as designed:
>> 
>> The producer will do
>>  /* code to set up cookies */
>>  wmb(); /* makes sure above changes are committed */
>>  d->hdr.state = VIO_DESC_READY;
>> 

Yeah, we need the producer memory barrier before setting
the descriptor state to ready. This ensures both data in the buffers
and other fields in the descriptor are flushed before the state
is visible to the consumer.  I do not believe the HV trap caused
by the trigger performs any sync of the store buffers.

>> the consumer will do
>> 
>>        if (desc->hdr.state != VIO_DESC_READY)
>>                return 1;
>>        err = vnet_rx_one(port, desc->size, desc->cookies, desc->ncookies);
>>                :
>>        desc->hdr.state = VIO_DESC_DONE;
>> 
>> So the vnet_rx_one() will only use valid cookie information at
>> all times.
>> 
>> This allows the code to correctly able to read multiple READY descriptors 
>> for a single LDC trigger, which it already does today.
>> (and it would be needlessly inefficient to clamp this down to
>> only one descriptor read per LDC-start in the vnet_rx())

Note, LDC message infrastructure is not a high performance infrastructure,
relying on an ldc message for each packet can greatly impact performance.
We need to reduce as many ldc messages as possible and rely on the
ring  more to pick up as many packets as possible without ldc messages.

The current implementation of vnet_start_xmit() performs all operations
such as grabbing a descriptor, copying the data into the data buffers
and updating the descriptor with in the same lock. This limits the parallelism
on the transmit path especially because the data copy operation is a bigger
operation and blocks other transmitters more than it is needed.  It is 
possible to split these operations where multiple transmitters can grab
descriptors in sequence but perform the copy operation in parallel.  When
this accomplished, there will be more possibility of racing with descriptors
getting ready quickly in a burst traffic situations. I believe the  udelay()
introduced in this patch will help even more when the transmitters are highly parallel.

-Raghuram
>> So what (if any) is the outstanding question about wmb() at this 
>> point?
>> 
>> --Sowmini
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> --
>> 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

--
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