Michael S. Tsirkin
2022-Aug-09 22:37 UTC
[virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets
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 at oracle.com> > > > > Sent: Tuesday, August 9, 2022 3:09 PM > > > > > > From: Si-Wei Liu <si-wei.liu at 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.> 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. > > -SiweiWeird 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.> > > > > > > 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 at lists.oasis-open.org > > For additional commands, e-mail: virtio-dev-help at lists.oasis-open.org > >
Si-Wei Liu
2022-Aug-09 22:54 UTC
[virtio-dev] [PATCH] virtio-net: use mtu size as buffer length for big packets
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 at oracle.com> >>>>> Sent: Tuesday, August 9, 2022 3:09 PM >>>>>>> From: Si-Wei Liu <si-wei.liu at 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> > >>> >>>> 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 at lists.oasis-open.org >>> For additional commands, e-mail: virtio-dev-help at lists.oasis-open.org >>>