Re: [PATCH net-next v4] net: virtio_net: notifications coalescing support

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 20, 2022 at 03:15:08PM +0800, Jason Wang wrote:
> On Wed, Jul 20, 2022 at 3:05 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 20, 2022 at 03:02:04PM +0800, Jason Wang wrote:
> > > On Wed, Jul 20, 2022 at 2:45 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Jul 19, 2022 at 05:26:52PM -0700, Jakub Kicinski wrote:
> > > > > On Mon, 18 Jul 2022 12:11:02 +0300 Alvaro Karsz wrote:
> > > > > > New VirtIO network feature: VIRTIO_NET_F_NOTF_COAL.
> > > > > >
> > > > > > Control a Virtio network device notifications coalescing parameters
> > > > > > using the control virtqueue.
> > > > > >
> > > > > > A device that supports this fetature can receive
> > > > > > VIRTIO_NET_CTRL_NOTF_COAL control commands.
> > > > > >
> > > > > > - VIRTIO_NET_CTRL_NOTF_COAL_TX_SET:
> > > > > >   Ask the network device to change the following parameters:
> > > > > >   - tx_usecs: Maximum number of usecs to delay a TX notification.
> > > > > >   - tx_max_packets: Maximum number of packets to send before a
> > > > > >     TX notification.
> > > > > >
> > > > > > - VIRTIO_NET_CTRL_NOTF_COAL_RX_SET:
> > > > > >   Ask the network device to change the following parameters:
> > > > > >   - rx_usecs: Maximum number of usecs to delay a RX notification.
> > > > > >   - rx_max_packets: Maximum number of packets to receive before a
> > > > > >     RX notification.
> > > > > >
> > > > > > VirtIO spec. patch:
> > > > > > https://lists.oasis-open.org/archives/virtio-comment/202206/msg00100.html
> > > > > >
> > > > > > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > > >
> > > > > Waiting a bit longer for Michael's ack, so in case other netdev
> > > > > maintainer takes this:
> > > > >
> > > > > Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> > > >
> > > > Yea was going to ack this but looking at the UAPI again we have a
> > > > problem because we abused tax max frames values 0 and 1 to control napi
> > > > in the past. technically does not affect legacy cards but userspace
> > > > can't easily tell the difference, can it?
> > >
> > > The "abuse" only works for iproute2.
> >
> > That's kernel/userspace API. That's what this patch affects, right?
> 
> I'm not sure I get this.
> 
> The 1-to-enable-napi is only used between iproute2 and kernel via
> ETHTOOL_A_COALESCE_TX_MAX_FRAMES not the uAPI introduced here.
> So I don't see how it can conflict with the virito uAPI extension here.
> 
> Thanks

As far as I can see ETHTOOL_A_COALESCE_TX_MAX_FRAMES invokes the
ops->get_coalesce and ops->set_coalesce callbacks.
This patch changes their behaviour when the card has VIRTIO_NET_F_NOTF_COAL.

Userspace making assumptions about what this option does will
thinkably might get unexpected behaviour. So:

Minimally we need a way for userspace to find out what are the semantics
of the command now, so one can implement portable userspace going
forward.

Preferably, analysis of existing userspace, what it does and how
does the change affect it should be included.

Ideally, a work-around that does not affect existing userspace
would be found.


> 
> >
> > > For uAPI we know it should follow
> > > the spec? (anyhow NAPI is something out of the spec)
> > >
> > > Thanks
> >
> > When you say uAPI here you mean the virtio header. I am not
> > worried about that just yet (maybe I should be).
> >
> > > >
> > > > --
> > > > MST
> > > >
> >