RE: [virtio-dev] [PATCH] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, February 6, 2023 5:27 PM

[..]
> > So, both can handle/generate spurious notifications, but it shouldn't be best
> line guidance.
> 
> Don't really see why not. Lots of spurious interrupts would defeat the purpose
> of the feature clearly. But it might be handy e.g. for migration purposes.
Why migration generate too many spurious interrupts?
May be one or two per vector at destination..

> Overall it's tough to document in detail but if there are too many interrupts
> then surely it's a quality of implementation issue.
> I feel trying to go into too much detail here will just pointlessly make it verbose
> without making it clearer.
A generic note that device can generate any number of spurious interrupts is not a spec material...

> > > what are packet counters though? they aren't defined anywhere.
> > >
> > Notification coalescing is counting packets in current text as " The device will
> count sent packets until it accumulates 15 ..."
> 
> so it says counts packets but it does not mean there's a counter.
Counting packet without a counter... interesting. :)
I see your point to better word it without "counter".

> And "meet" does not really work with "parameters"...
> 
A better wording is necessary.

> > > > For example, current \field{max_packets} is 15 for transmit
> > > > virtqueue, and
> > > device already sent 10 packets.
> > >
> > > I assume it is all per virtqueue? Makes sense but it does not
> > > actually say anywhere.
> > >
> > Yes, it is per virtqueue because the coalescing commands are per virtqueue.
> 
> in what sense? there's a single set of parameters per device.
> \begin{lstlisting}
> struct virtio_net_ctrl_coal_rx {
>     le32 rx_max_packets;
>     le32 rx_usecs;
> };
> 
> struct virtio_net_ctrl_coal_tx {
>     le32 tx_max_packets;
>     le32 tx_usecs;
> };
> 
> #define VIRTIO_NET_CTRL_NOTF_COAL 6
>  #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0  #define
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1 \end{lstlisting}
> 
> does not specify to which vq this applies, I think the implication is it applies to
> all of them.
> 
Sadly yes.
I missed this basic thing missing in spec.
Even 5 years old algorithm like net dim may not work effectively with device global coalescing parameters.
Since 1.3 is not released, lets please fix it now to have it per VQ to avoid having yet another command.
A tool that relies on global device level will still work with per VQ level too.

> 
> 
> > Not sure explicitly keep adding per virtqueue in every line improves the
> readability.
> 
> well we have to say this somewhere at least and AFAIK we don't.
> 
> > > > When device receives a VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> command,
> > > the device SHOULD immediately send a TX notification.
> > >
> > > always? why?
> > Ah no. not always. It was in the context of the example continuation.
> >
> > May be better to rewrite as,
> >
> > For example, current \field{max_packets} is 15 for the transmit virtqueue,
> and device already sent 10 packets; in such case when device receives a
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, the device SHOULD
> immediately send a TX notification.
> 
> 
> Parav this is not the best way to give comments I feel. Either just send a
> competing patch or (preferably) explain what is wrong with this one.
> Piecemeal "Its better written as" without anything in the way of explanation is
> not helpful. I for one don't have the time to review and reply to not just patches
> but also all the random suggestions you are throwing around.

It is not random and not "all".
The part I missed is current command limits to per device.
I didn't expect VQN to be missing. :(

Please don't attribute suggestion of "avoid device is good to generate spurious interrupt" as random..

I understood your suggestion that when suggesting, describe the problem and the solution both.
I will improve this area going forward. Thanks for the inputs.