[RFC] use smp_load_acquire()/smp_store_release()

[RFC] use smp_load_acquire()/smp_store_release()

[Eric Dumazet]

Hi Alexander

The memory barriers added in commit
b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
("net: Add memory barriers to prevent possible race in byte queue
limits")

have heavy cost.

It seems we could use smp_load_acquire() and smp_store_release()
instead ?

I'll post a patch later today. I would be interested if someone was able
to test it, as your commit apparently was tested and known to fix a
reproducible race.

Thanks !

Re: [RFC] use smp_load_acquire()/smp_store_release()

[Alexander Duyck]

On 10/29/2014 07:49 AM, Eric Dumazet wrote:
> Hi Alexander
>
> The memory barriers added in commit
> b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
> ("net: Add memory barriers to prevent possible race in byte queue
> limits")
>
> have heavy cost.
>
> It seems we could use smp_load_acquire() and smp_store_release()
> instead ?
>
> I'll post a patch later today. I would be interested if someone was able
> to test it, as your commit apparently was tested and known to fix a
> reproducible race.
>
> Thanks !

Unfortunately Stephen left Intel before I did, so we will need to find 
someone else in the validation team to test this if possible. I have 
added Jeff to the CC so that he can give the appropriate validation 
people a heads up that this patch might be coming.

As I recall what was seen was random Tx hangs on systems with the 
original BQL code when interfaces were stressed.  It has been a while so 
I don't recall the exact set-up for all of it.  Also some less 
used/tested architectures such as PowerPC can be more susceptible to 
synchronization issues such as these as the memory model is more weakly 
ordered.

I'm wondering where you are seeing the barrier show up?  In 
netdev_tx_send_queue you should only hit the barrier if you actually are 
triggering the XOFF condition, and in netdev_tx_completed_queue the 
barrier should be coalesced in amongst a number of frames reducing the cost.

My concern with this would be that we are actually syncronizing multiple 
things, the __QUEUE_STATE_STACK_XOFF flag, dql->adj_limit, and 
dql->num_queued, and we might be trading off reducing the cost on x86 to 
result in it being increased on other architectures as we may have to 
actually add additional synchronization as I suspect we would need to 
use acquire/release on both adj_limit and num_queued.

Thanks,

Alex

Re: [RFC] use smp_load_acquire()/smp_store_release()

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 2147 bytes --]

On Wed, 2014-10-29 at 09:16 -0700, Alexander Duyck wrote:
> On 10/29/2014 07:49 AM, Eric Dumazet wrote:
> > Hi Alexander
> >
> > The memory barriers added in commit
> > b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
> > ("net: Add memory barriers to prevent possible race in byte queue
> > limits")
> >
> > have heavy cost.
> >
> > It seems we could use smp_load_acquire() and smp_store_release()
> > instead ?
> >
> > I'll post a patch later today. I would be interested if someone was able
> > to test it, as your commit apparently was tested and known to fix a
> > reproducible race.
> >
> > Thanks !

Eric- just CC me on the patch you post and I will see what I can do
about getting validation eyes on it.

> 
> Unfortunately Stephen left Intel before I did, so we will need to find 
> someone else in the validation team to test this if possible. I have 
> added Jeff to the CC so that he can give the appropriate validation 
> people a heads up that this patch might be coming.
> 
> As I recall what was seen was random Tx hangs on systems with the 
> original BQL code when interfaces were stressed.  It has been a while so 
> I don't recall the exact set-up for all of it.  Also some less 
> used/tested architectures such as PowerPC can be more susceptible to 
> synchronization issues such as these as the memory model is more weakly 
> ordered.
> 
> I'm wondering where you are seeing the barrier show up?  In 
> netdev_tx_send_queue you should only hit the barrier if you actually are 
> triggering the XOFF condition, and in netdev_tx_completed_queue the 
> barrier should be coalesced in amongst a number of frames reducing the cost.
> 
> My concern with this would be that we are actually syncronizing multiple 
> things, the __QUEUE_STATE_STACK_XOFF flag, dql->adj_limit, and 
> dql->num_queued, and we might be trading off reducing the cost on x86 to 
> result in it being increased on other architectures as we may have to 
> actually add additional synchronization as I suspect we would need to 
> use acquire/release on both adj_limit and num_queued.
> 
> Thanks,
> 
> Alex
> 



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC] use smp_load_acquire()/smp_store_release()

[Eric Dumazet]

On Wed, 2014-10-29 at 12:27 -0700, Jeff Kirsher wrote:
> On Wed, 2014-10-29 at 09:16 -0700, Alexander Duyck wrote:
> > On 10/29/2014 07:49 AM, Eric Dumazet wrote:
> > > Hi Alexander
> > >
> > > The memory barriers added in commit
> > > b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
> > > ("net: Add memory barriers to prevent possible race in byte queue
> > > limits")
> > >
> > > have heavy cost.
> > >
> > > It seems we could use smp_load_acquire() and smp_store_release()
> > > instead ?
> > >
> > > I'll post a patch later today. I would be interested if someone was able
> > > to test it, as your commit apparently was tested and known to fix a
> > > reproducible race.
> > >
> > > Thanks !
> 
> Eric- just CC me on the patch you post and I will see what I can do
> about getting validation eyes on it.

Thanks guys, will do, and will CC Paul as well.

Alexander, here is the following profile showing the cost of the
'mfence', in a typical rpc workload (a lot of IRQ are generated for TX
completions, because RPC tend to send small packets)

  0.11 │       je     33a
       │       mov    -0x3c(%rbp),%esi
  0.06 │       lea    0xc0(%rbx),%rdi
  0.06 │       callq  dql_completed
  0.06 │       mfence
 38.68 │       mov    0xc4(%rbx),%edx
  1.83 │       mov    0xc0(%rbx),%eax
       │       cmp    %eax,%edx
  0.22 │       js     333
  0.11 │       lock   btrl $0x1,0x98(%rbx)

Re: [RFC] use smp_load_acquire()/smp_store_release()

[Alexander Duyck]

On 10/29/2014 12:57 PM, Eric Dumazet wrote:
> On Wed, 2014-10-29 at 12:27 -0700, Jeff Kirsher wrote:
>> On Wed, 2014-10-29 at 09:16 -0700, Alexander Duyck wrote:
>>> On 10/29/2014 07:49 AM, Eric Dumazet wrote:
>>>> Hi Alexander
>>>>
>>>> The memory barriers added in commit
>>>> b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
>>>> ("net: Add memory barriers to prevent possible race in byte queue
>>>> limits")
>>>>
>>>> have heavy cost.
>>>>
>>>> It seems we could use smp_load_acquire() and smp_store_release()
>>>> instead ?
>>>>
>>>> I'll post a patch later today. I would be interested if someone was able
>>>> to test it, as your commit apparently was tested and known to fix a
>>>> reproducible race.
>>>>
>>>> Thanks !
>> Eric- just CC me on the patch you post and I will see what I can do
>> about getting validation eyes on it.
> Thanks guys, will do, and will CC Paul as well.
>
> Alexander, here is the following profile showing the cost of the
> 'mfence', in a typical rpc workload (a lot of IRQ are generated for TX
> completions, because RPC tend to send small packets)
>
>    0.11 │       je     33a
>         │       mov    -0x3c(%rbp),%esi
>    0.06 │       lea    0xc0(%rbx),%rdi
>    0.06 │       callq  dql_completed
>    0.06 │       mfence
>   38.68 │       mov    0xc4(%rbx),%edx
>    1.83 │       mov    0xc0(%rbx),%eax
>         │       cmp    %eax,%edx
>    0.22 │       js     333
>    0.11 │       lock   btrl $0x1,0x98(%rbx)

It might be worthwhile to see if it would be possible to combine BQL 
with the mechanism the drivers have for handling descriptors/packets.  
Otherwise you are going to be pulling one barrier just to hit another 
right after it.

Also depending on what driver it is that the trace is from you may want 
to check and see if you have any MMIO transactions occurring right 
before you make the call, otherwise that may be the actual cause for the 
significant cost as you are having to flush non-coherent memory before 
you can resume operation.

Thanks,

Alex