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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Thu, Sep 13, 2018 at 5:02 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> 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().

The first iteration of my patch checked IFF_UP and effectively
only allowed the change when not running. What do you mean
by need to check?

And to respond to the other follow-up notes at once:

> Consider we may have interrupt moderation in the future, I tend to use
> set_coalesce. Otherwise we may need two steps to enable moderation:
>
> - tx-napi on
> - set_coalesce

FWIW, I don't care strongly whether we do this through coalesce or priv_flags.

>> +                     if (!napi_weight)
>> +                             virtqueue_enable_cb(vi->sq[i].vq);
>
> I don't get why we need to disable enable cb here.

To avoid entering no-napi mode with too few descriptors to
make progress and no way to get out of that state. This is a
pretty crude attempt at handling that, admittedly.