Re: [PATCH] virtio-net: Ignore num_buffers when unused

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

On Thu, Dec 26, 2024 at 04:35:55PM +0900, Akihiko Odaki wrote:
> On 2024/12/26 16:32, Michael S. Tsirkin wrote:
> > On Thu, Dec 26, 2024 at 02:29:09AM -0500, Michael S. Tsirkin wrote:
> > > On Thu, Dec 26, 2024 at 02:37:17PM +0900, Akihiko Odaki wrote:
> > > > If VIRTIO_NET_F_MRG_RXBUF has not been negotuated, vhost_net and QEMU
> > > > sets num_buffers to zero while the specification tells to set it to one.
> > > > 
> > > > Let drivers ignore the field to make them compatible with both
> > > > vhost_net/QEMU and devices already compliant with the specification.
> > > > Also, remove statements requiring devices to set the field to one to
> > > > make vhost_net and QEMU compliant.
> > > > 
> > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > > ---
> > > >   device-types/net/description.tex | 26 +++++++++++---------------
> > > >   1 file changed, 11 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > > > index 76585b0..b82fe8f 100644
> > > > --- a/device-types/net/description.tex
> > > > +++ b/device-types/net/description.tex
> > > > @@ -683,15 +683,12 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
> > > >   \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}
> > > > -The device MUST set \field{num_buffers} to the number of descriptors used to
> > > > -hold the incoming packet.
> > > > +If VIRTIO_NET_F_MRG_RXBUF was negotiated, the device MUST set
> > > > +\field{num_buffers} to the number of descriptors used to hold the incoming
> > > > +packet.
> > > >   The device MUST use only a single descriptor if VIRTIO_NET_F_MRG_RXBUF
> > > >   was not negotiated.
> > > > -\begin{note}
> > > > -{This means that \field{num_buffers} will always be 1
> > > > -if VIRTIO_NET_F_MRG_RXBUF is not negotiated.}
> > > > -\end{note}
> > > >   \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets}
> > > >   \label{sec:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff
> > > > @@ -704,9 +701,8 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > > >   Processing incoming packets involves:
> > > >   \begin{enumerate}
> > > > -\item \field{num_buffers} indicates how many descriptors
> > > > -  this packet is spread over (including this one): this will
> > > > -  always be 1 if VIRTIO_NET_F_MRG_RXBUF was not negotiated.
> > > > +\item If VIRTIO_NET_F_MRG_RXBUF was negogiated, \field{num_buffers} indicates
> > > > +  how many descriptors this packet is spread over (including this one).
> > > >     This allows receipt of large packets without having to allocate large
> > > >     buffers: a packet that does not fit in a single buffer can flow
> > > >     over to the next buffer, and so on. In this case, there will be
> > > > @@ -717,9 +713,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > > >     The other buffers will not begin with a struct virtio_net_hdr.
> > > >   \item If
> > > > -  \field{num_buffers} is one, then the entire packet will be
> > > > -  contained within this buffer, immediately following the struct
> > > > -  virtio_net_hdr.
> > > > +  VIRTIO_NET_F_MRG_RXBUF was not negotiated or \field{num_buffers} is one, then
> > > > +  the entire packet will be contained within this buffer, immediately following
> > > > +  the struct virtio_net_hdr.
> > > >   \item If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> > > >     VIRTIO_NET_HDR_F_DATA_VALID bit in \field{flags} can be
> > > >     set: if so, device has validated the packet checksum.
> > > > @@ -764,9 +760,6 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > > >   \devicenormative{\paragraph}{Processing of Incoming Packets}{Device Types / Network Device / Device Operation / Processing of Incoming Packets}
> > > >   \label{devicenormative:Device Types / Network Device / Device Operation / Processing of Packets}%old label for latexdiff
> > > > -If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the device MUST set
> > > > -\field{num_buffers} to 1.
> > > > -
> > > >   If VIRTIO_NET_F_MRG_RXBUF has been negotiated, the device MUST set
> > > >   \field{num_buffers} to indicate the number of buffers
> > > >   the packet (including the header) is spread over.
> > > > @@ -863,6 +856,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
> > > >   This is due to various bugs in implementations.
> > > >   \end{note}
> > > > +If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST ignore
> > > > +\field{num_buffers}.
> > > > +
> > > >   If neither VIRTIO_NET_HDR_F_NEEDS_CSUM nor
> > > >   VIRTIO_NET_HDR_F_DATA_VALID is set, the driver MUST NOT
> > > >   rely on the packet checksum being correct.
> > > 
> > > 
> > > There could be drivers relying on it.
> > > We can add a SHOULD not and ask drivers not to look at it,
> 
> The patch adds the following statement:
> > If VIRTIO_NET_F_MRG_RXBUF has not been negotiated, the driver MUST
> > ignore \field{num_buffers}.
> 
> Do you suggest changing MUST to SHOULD or rephrase other words in this
> statement?

Yes, I would change MUST to SHOULD. Basically documenting the existing
practice.

But I do not yes see why we should drop the device requirements the way you suggest.
We just need to fix devices, it does not seem hard.

> > > and we should fix devices.
> > 
> > Maybe allowing 0 too is a good idea? Need to think through
> > implemenation concerns.
> > In any case, pls fix devices.
> 
> I opted allowing any values for devices based on the discussion in the
> following thread:
> https://lore.kernel.org/virtio-comment/CY8PR12MB7195E5A1D321CABBA97D3FD7DC0D2@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5789536fe7405a7be18e167ff522c27333656fe9
> 
> Regards,
> Akihiko Odaki

That one asks for concrete data.

-- 
MST