sunvnet: Question on tx control in original code

sunvnet: Question on tx control in original code

[Shannon Nelson]

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

Re: sunvnet: Question on tx control in original code

[Sowmini Varadhan]

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

Re: sunvnet: Question on tx control in original code

[David Miller]

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.

Re: sunvnet: Question on tx control in original code

[Shannon Nelson]

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