Re: [virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets

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

On Tue, Aug 09, 2022 at 03:54:57PM -0700, Si-Wei Liu wrote:
> 
> 
> On 8/9/2022 3:37 PM, Michael S. Tsirkin wrote:
> > On Tue, Aug 09, 2022 at 03:32:26PM -0700, Si-Wei Liu wrote:
> > > 
> > > On 8/9/2022 2:37 PM, Michael S. Tsirkin wrote:
> > > > On Tue, Aug 09, 2022 at 07:18:30PM +0000, Parav Pandit wrote:
> > > > > > From: Si-Wei Liu <si-wei.liu@oracle.com>
> > > > > > Sent: Tuesday, August 9, 2022 3:09 PM
> > > > > > > > From: Si-Wei Liu <si-wei.liu@oracle.com>
> > > > > > > > Sent: Tuesday, August 9, 2022 2:39 PM Currently it is not. Not a
> > > > > > > > single patch nor this patch, but the context for the eventual goal is
> > > > > > > > to allow XDP on a MTU=9000 link when guest users intentionally lower
> > > > > > > > down MTU to 1500.
> > > > > > > Which application benefit by having asymmetry by lowering mtu to 1500
> > > > > > to send packets but want to receive 9K packets?
> > > > > Below details doesnât answer the question of asymmetry. :)
> > > > > 
> > > > > > I think virtio-net driver doesn't differentiate MTU and MRU, in which case
> > > > > > the receive buffer will be reduced to fit the 1500B payload size when mtu is
> > > > > > lowered down to 1500 from 9000.
> > > > > How? Driver reduced the mXu to 1500, say it is improved to post buffers of 1500 bytes.
> > > > > 
> > > > > Device doesn't know about it because mtu in config space is RO field.
> > > > > Device keep dropping 9K packets because buffers posted are 1500 bytes.
> > > > > This is because device follows the spec " The device MUST NOT pass received packets that exceed mtu".
> > > > The "mtu" here is the device config field, which is
> > > > 
> > > >           /* Default maximum transmit unit advice */
> > > > 
> > > > there is no guarantee device will not get a bigger packet.
> > > > And there is no guarantee such a packet will be dropped
> > > > as opposed to wedging the device if userspace insists on
> > > > adding smaller buffers.
> > > It'd be nice to document this requirement or statement to the spec for
> > > clarity purpose.
> > It's not a requirement, more of a bug. But it's been like this for
> > years.
> Well, I'm not sure how it may wedge the device if not capable of posting to
> smaller buffers, is there other option than drop? Truncate to what the
> buffer size may fit and deliver up? Seems even worse than drop...
> 
> > 
> > > Otherwise various device implementations are hard to
> > > follow. The capture is that vhost-net drops bigger packets while the driver
> > > only supplied smaller buffers. This is the status quo, and users seemingly
> > > have relied on this behavior for some while.
> > > 
> > > -Siwei
> > Weird where do you see this in code? I see
> > 
> >                  sock_len = vhost_net_rx_peek_head_len(net, sock->sk,
> >                                                        &busyloop_intr);
> >                  if (!sock_len)
> >                          break;
> >                  sock_len += sock_hlen;
> >                  vhost_len = sock_len + vhost_hlen;
> >                  headcount = get_rx_bufs(vq, vq->heads + nvq->done_idx,
> >                                          vhost_len, &in, vq_log, &log,
> >                                          likely(mergeable) ? UIO_MAXIOV : 1);
> >                  /* On error, stop handling until the next kick. */
> >                  if (unlikely(headcount < 0))
> >                          goto out;
> > 
> > 
> > so it does not drop a packet, it just stops processing the queue.
> Here
> 
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ /* On overrun, truncate and discard */
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (unlikely(headcount > UIO_MAXIOV)) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ err = sock->ops->recvmsg(sock, &msg,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 1, MSG_DONTWAIT |
> MSG_TRUNC);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_debug("Discarded rx packet: len %zd\n",
> sock_len);
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
> 
> recvmsg(, , 1, ) is essentially to drop the oversized packet.
> 
> 
> In get_rx_bufs(), overrun detection will return something larger than
> UIO_MAXIOV as indicator:
> 
> static int get_rx_bufs()
> {
> :
> ;
> ÂÂÂÂÂÂÂ /* Detect overrun */
> ÂÂÂÂÂÂÂ if (unlikely(datalen > 0)) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ r = UIO_MAXIOV + 1;
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ goto err;
> ÂÂÂÂÂÂÂ }
> :
> :
> 
> 
> -Siwei


Hmm you are right. I'll check but it seems I have misread the code.
Sorry about wasting your time on this.
So maybe the approach is ok then.
It's late, I'll recheck tomorrow.


> > 
> > 
> > > > 
> > > > > So, I am lost what virtio net device user application is trying to achieve by sending smaller packets and dropping all receive packets.
> > > > > (it doesnât have any relation to mergeable or otherwise).
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > > >