Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570035 > unrolled thread

Re: [PATCH] xen-netfront: Delete rx_refill_timer in xennet_disconnect_backend()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] xen-netfront: Delete rx_refill_timer in  xennet_disconnect_backend() Eric Dumazet <eric.dumazet@gmail.com> - 2017-01-30 20:10 +0100
    Re: [PATCH] xen-netfront: Delete rx_refill_timer in  xennet_disconnect_backend() Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-01-30 20:40 +0100
      Re: [PATCH] xen-netfront: Delete rx_refill_timer in  xennet_disconnect_backend() Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-01-31 18:50 +0100
        Re: [PATCH] xen-netfront: Delete rx_refill_timer in  xennet_disconnect_backend() Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-02-02 00:30 +0100
          Re: [PATCH] xen-netfront: Delete rx_refill_timer in  xennet_disconnect_backend() Eric Dumazet <eric.dumazet@gmail.com> - 2017-02-02 01:10 +0100

#1570035 — Re: [PATCH] xen-netfront: Delete rx_refill_timer in xennet_disconnect_backend()

On Mon, 2017-01-30 at 13:23 -0500, Boris Ostrovsky wrote:

> We do netif_carrier_off() first thing in xennet_disconnect_backend() and
> the only place where the timer is rearmed is xennet_alloc_rx_buffers(),
> which is guarded by netif_carrier_ok() check.

Oh well, testing netif_carrier_ok() in packet processing fast path looks
unusual and a waste of cpu cycles. I've never seen that pattern before.

If one day, we remove this netif_carrier_ok() test during a cleanup,
then the race window will open again.

[toc] | [next] | [standalone]

#1570060

On 01/30/2017 02:06 PM, Eric Dumazet wrote:
> On Mon, 2017-01-30 at 13:23 -0500, Boris Ostrovsky wrote:
>
>> We do netif_carrier_off() first thing in xennet_disconnect_backend() and
>> the only place where the timer is rearmed is xennet_alloc_rx_buffers(),
>> which is guarded by netif_carrier_ok() check.
> Oh well, testing netif_carrier_ok() in packet processing fast path looks
> unusual and a waste of cpu cycles. I've never seen that pattern before.
>
> If one day, we remove this netif_carrier_ok() test during a cleanup,
> then the race window will open again.


I don't know much about napi but I wonder whether I can indeed disable
it in xennet_disconnect_backend(). I don't see how anything can happen
after disconnect since it unmaps the rings. And then napi is re-enabled
during reconnection in xennet_create_queues(). In which case am not sure
there is any need for xennet_destroy_queues() as everything there could
be folded into xennet_disconnect_backend().

-boris

[toc] | [prev] | [next] | [standalone]

#1570941

On 01/30/2017 02:31 PM, Boris Ostrovsky wrote:
> On 01/30/2017 02:06 PM, Eric Dumazet wrote:
>> On Mon, 2017-01-30 at 13:23 -0500, Boris Ostrovsky wrote:
>>
>>> We do netif_carrier_off() first thing in xennet_disconnect_backend() and
>>> the only place where the timer is rearmed is xennet_alloc_rx_buffers(),
>>> which is guarded by netif_carrier_ok() check.
>> Oh well, testing netif_carrier_ok() in packet processing fast path looks
>> unusual and a waste of cpu cycles. I've never seen that pattern before.
>>
>> If one day, we remove this netif_carrier_ok() test during a cleanup,
>> then the race window will open again.
>
> I don't know much about napi but I wonder whether I can indeed disable
> it in xennet_disconnect_backend(). I don't see how anything can happen
> after disconnect since it unmaps the rings. And then napi is re-enabled
> during reconnection in xennet_create_queues(). In which case am not sure
> there is any need for xennet_destroy_queues() as everything there could
> be folded into xennet_disconnect_backend().

While this does work, there was a reason why napi_disable() was not
called in xennet_disconnect_backend() and it is explained in commit
ce58725fec6e --- napi_disable() may sleep and that's why it is called in
xennet_destroy_queues().

OTOH, there is a napi_synchronize() call in xennet_destroy_queues().
Will destroying the timer after it guarantee that all preceding RX have
been completed? RX interrupt is disabled prior to napi_synchronize() so
presumably nothing new can be received.


-boris

[toc] | [prev] | [next] | [standalone]

#1572048

On 01/31/2017 12:47 PM, Boris Ostrovsky wrote:
> On 01/30/2017 02:31 PM, Boris Ostrovsky wrote:
>> On 01/30/2017 02:06 PM, Eric Dumazet wrote:
>>> On Mon, 2017-01-30 at 13:23 -0500, Boris Ostrovsky wrote:
>>>
>>>> We do netif_carrier_off() first thing in xennet_disconnect_backend() and
>>>> the only place where the timer is rearmed is xennet_alloc_rx_buffers(),
>>>> which is guarded by netif_carrier_ok() check.
>>> Oh well, testing netif_carrier_ok() in packet processing fast path looks
>>> unusual and a waste of cpu cycles. I've never seen that pattern before.
>>>
>>> If one day, we remove this netif_carrier_ok() test during a cleanup,
>>> then the race window will open again.
>> I don't know much about napi but I wonder whether I can indeed disable
>> it in xennet_disconnect_backend(). I don't see how anything can happen
>> after disconnect since it unmaps the rings. And then napi is re-enabled
>> during reconnection in xennet_create_queues(). In which case am not sure
>> there is any need for xennet_destroy_queues() as everything there could
>> be folded into xennet_disconnect_backend().
> While this does work, there was a reason why napi_disable() was not
> called in xennet_disconnect_backend() and it is explained in commit
> ce58725fec6e --- napi_disable() may sleep and that's why it is called in
> xennet_destroy_queues().
>
> OTOH, there is a napi_synchronize() call in xennet_destroy_queues().
> Will destroying the timer after it guarantee that all preceding RX have
> been completed? RX interrupt is disabled prior to napi_synchronize() so
> presumably nothing new can be received.


I could not convince myself that napi_synchronize() is sufficient here
(mostly because I am not familiar with napi flow). At the same time I
would rather not make changes in anticipation of possible disappearance
of netif_carrier_ok() in the future so I'd like this patch to go in as is.

Unless there are other problems with the patch or if Eric (or others)
feel strongly about usage of netif_carrier_ok() here.


-boris

[toc] | [prev] | [next] | [standalone]

#1572084

On Wed, 2017-02-01 at 18:29 -0500, Boris Ostrovsky wrote:

> 
> I could not convince myself that napi_synchronize() is sufficient here
> (mostly because I am not familiar with napi flow). At the same time I
> would rather not make changes in anticipation of possible disappearance
> of netif_carrier_ok() in the future so I'd like this patch to go in as is.
> 
> Unless there are other problems with the patch or if Eric (or others)
> feel strongly about usage of netif_carrier_ok() here.
> 

No strong feelings from me.
We probably have more serious issues to fix anyway.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web