Re: [PATCH net-next] virtio_net: ethtool tx napi configuration

[Jason Wang <jasowang@redhat.com>]

On 2018年09月13日 07:27, Willem de Bruijn wrote:
> On Wed, Sep 12, 2018 at 3:11 PM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com> wrote:
>> On Wed, Sep 12, 2018 at 2:16 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>
>>>
>>> On 9/12/2018 11:07 AM, Willem de Bruijn wrote:
>>>> On Wed, Sep 12, 2018 at 1:42 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>>>>
>>>>>
>>>>> On 9/9/2018 3:44 PM, Willem de Bruijn wrote:
>>>>>> From: Willem de Bruijn <willemb@google.com>
>>>>>>
>>>>>> Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers.
>>>>>> Interrupt moderation is currently not supported, so these accept and
>>>>>> display the default settings of 0 usec and 1 frame.
>>>>>>
>>>>>> Toggle tx napi through a bit in tx-frames. So as to not interfere
>>>>>> with possible future interrupt moderation, use bit 10, well outside
>>>>>> the reasonable range of real interrupt moderation values.
>>>>>>
>>>>>> Changes are not atomic. The tx IRQ, napi BH and transmit path must
>>>>>> be quiesced when switching modes. Only allow changing this setting
>>>>>> when the device is down.
>>>>> Humm, would not a private ethtool flag to switch TX NAPI on/off be more
>>>>> appropriate rather than use the coalescing configuration API here?
>>>> What do you mean by private ethtool flag? A new field in ethtool
>>>> --features (-k)?
>>> I meant using ethtool_drvinfo::n_priv_flags, ETH_SS_PRIV_FLAGS and then
>>> ETHTOOL_GFPFLAGS and ETHTOOL_SPFLAGS to control the toggling of that
>>> private flag. mlx5 has a number of privates flags for instance.
>> Interesting, thanks! I was not at all aware of those ethtool flags.
>> Am having a look. It definitely looks promising.
> Okay, I made that change. That is indeed much cleaner, thanks.
> Let me send the patch, initially as RFC.
>
> I've observed one issue where if we toggle the flag before bringing
> up the device, it hits a kernel BUG at include/linux/netdevice.h:515
>
>          BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));

This reminds me that we need to check netif_running() before trying to 
enable and disable tx napi in ethtool_set_coalesce().

Thanks