* sunvnet: Question on tx control in original code
@ 2016-12-01 1:25 Shannon Nelson
2016-12-01 2:41 ` Sowmini Varadhan
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Shannon Nelson @ 2016-12-01 1:25 UTC (permalink / raw)
To: sparclinux
I've recently been studying up on the sparc LDom environment and
sunvnet, the virtual network device. In looking through the code I ran
across an odd bit of tx control that I don't quite understand.
Near the bottom of sunvnet_common.c:sunvnet_start_xmit_common(), about
line 1423, just after the ldc_start_done: label is a curious 'if' block
if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
netif_tx_stop_queue(txq);
if (vnet_tx_dring_avail(dr) > VNET_TX_WAKEUP_THRESH(dr))
netif_tx_wake_queue(txq);
}
In order to get into the if-block there needs to be a 0 or negative
return from vnet_tx_dring_avail(), yet then we're checking it again,
with no discernible delay, for a positive value. I don't see how we
could get to the netif_tx_wake_queue() call - shouldn't there be some
delay, or perhaps something that might bump the underlying channel
before checking for more available space?
Perhaps I've missed something?
Thanks,
sln
--
=========================
Shannon Nelson shannon.nelson@oracle.com
Parents can't afford to be squeamish
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sunvnet: Question on tx control in original code
2016-12-01 1:25 sunvnet: Question on tx control in original code Shannon Nelson
@ 2016-12-01 2:41 ` Sowmini Varadhan
2016-12-02 17:31 ` David Miller
2016-12-02 17:46 ` Shannon Nelson
2 siblings, 0 replies; 4+ messages in thread
From: Sowmini Varadhan @ 2016-12-01 2:41 UTC (permalink / raw)
To: sparclinux
On (11/30/16 17:25), Shannon Nelson wrote:
> I've recently been studying up on the sparc LDom environment and
> sunvnet, the virtual network device. In looking through the code I
> ran across an odd bit of tx control that I don't quite understand.
:
> In order to get into the if-block there needs to be a 0 or negative
> return from vnet_tx_dring_avail(), yet then we're checking it again,
> with no discernible delay, for a positive value. I don't see how we
> could get to the netif_tx_wake_queue() call - shouldn't there be
> some delay, or perhaps something that might bump the underlying
> channel before checking for more available space?
>
> Perhaps I've missed something?
And fwiw, Shannon asked me this question, and I too did not have the
history here. Perhaps DaveM remembers where this came from?
--Sowmini
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sunvnet: Question on tx control in original code
2016-12-01 1:25 sunvnet: Question on tx control in original code Shannon Nelson
2016-12-01 2:41 ` Sowmini Varadhan
@ 2016-12-02 17:31 ` David Miller
2016-12-02 17:46 ` Shannon Nelson
2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2016-12-02 17:31 UTC (permalink / raw)
To: sparclinux
From: Shannon Nelson <shannon.nelson@oracle.com>
Date: Wed, 30 Nov 2016 17:25:30 -0800
> Near the bottom of sunvnet_common.c:sunvnet_start_xmit_common(), about
> line 1423, just after the ldc_start_done: label is a curious 'if'
> block
>
> if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
> netif_tx_stop_queue(txq);
> if (vnet_tx_dring_avail(dr) > VNET_TX_WAKEUP_THRESH(dr))
> netif_tx_wake_queue(txq);
> }
>
> In order to get into the if-block there needs to be a 0 or negative
> return from vnet_tx_dring_avail(), yet then we're checking it again,
> with no discernible delay, for a positive value. I don't see how we
> could get to the netif_tx_wake_queue() call - shouldn't there be some
> delay, or perhaps something that might bump the underlying channel
> before checking for more available space?
This is the canonical way to deal with a race condition that exists in
pretty much every modern networking driver that does transmit without
holding onto any real locks.
Between the initial vnet_tx_dring_avail() test and the
netif_tx_stop_queue() call, a TX event can arrive and thus make TX
ring space available again.
Thus without the re-check, we could lose a wakeup.
If you scan a bunch of drivers, you'll find this construct is common,
for example tg3_start_xmit() has:
tnapi->tx_prod = entry;
if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) {
netif_tx_stop_queue(txq);
/* netif_tx_stop_queue() must be done before checking
* checking tx index in tg3_tx_avail() below, because in
* tg3_tx(), we update tx index before checking for
* netif_tx_queue_stopped().
*/
smp_mb();
if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
netif_tx_wake_queue(txq);
}
Hope this helps.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: sunvnet: Question on tx control in original code
2016-12-01 1:25 sunvnet: Question on tx control in original code Shannon Nelson
2016-12-01 2:41 ` Sowmini Varadhan
2016-12-02 17:31 ` David Miller
@ 2016-12-02 17:46 ` Shannon Nelson
2 siblings, 0 replies; 4+ messages in thread
From: Shannon Nelson @ 2016-12-02 17:46 UTC (permalink / raw)
To: sparclinux
On 12/2/2016 9:31 AM, David Miller wrote:
> From: Shannon Nelson <shannon.nelson@oracle.com>
> Date: Wed, 30 Nov 2016 17:25:30 -0800
>
>> Near the bottom of sunvnet_common.c:sunvnet_start_xmit_common(), about
>> line 1423, just after the ldc_start_done: label is a curious 'if'
>> block
>>
>> if (unlikely(vnet_tx_dring_avail(dr) < 1)) {
>> netif_tx_stop_queue(txq);
>> if (vnet_tx_dring_avail(dr) > VNET_TX_WAKEUP_THRESH(dr))
>> netif_tx_wake_queue(txq);
>> }
>>
>> In order to get into the if-block there needs to be a 0 or negative
>> return from vnet_tx_dring_avail(), yet then we're checking it again,
>> with no discernible delay, for a positive value. I don't see how we
>> could get to the netif_tx_wake_queue() call - shouldn't there be some
>> delay, or perhaps something that might bump the underlying channel
>> before checking for more available space?
>
> This is the canonical way to deal with a race condition that exists in
> pretty much every modern networking driver that does transmit without
> holding onto any real locks.
>
> Between the initial vnet_tx_dring_avail() test and the
> netif_tx_stop_queue() call, a TX event can arrive and thus make TX
> ring space available again.
>
> Thus without the re-check, we could lose a wakeup.
>
> If you scan a bunch of drivers, you'll find this construct is common,
> for example tg3_start_xmit() has:
>
> tnapi->tx_prod = entry;
> if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) {
> netif_tx_stop_queue(txq);
>
> /* netif_tx_stop_queue() must be done before checking
> * checking tx index in tg3_tx_avail() below, because in
> * tg3_tx(), we update tx index before checking for
> * netif_tx_queue_stopped().
> */
> smp_mb();
> if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
> netif_tx_wake_queue(txq);
> }
>
> Hope this helps.
>
Yes, thanks, this helps. I note that in your example there is an
smp_mb() call before the second check for available Tx - a potential
delayer that would help widen the window for something to happen in the
background. This is the kind of thing I was suspecting was missing from
the sunvnet code. I don't know the sparc architecture very well yet,
but I suspect this would not be a bad thing to add to the sunvnet code.
It's probably worth us doing a little checking to see how often we might
end up there under a load.
sln
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-02 17:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-01 1:25 sunvnet: Question on tx control in original code Shannon Nelson
2016-12-01 2:41 ` Sowmini Varadhan
2016-12-02 17:31 ` David Miller
2016-12-02 17:46 ` Shannon Nelson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.