Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum

[Jason Wang <jasowang@redhat.com>]

On Thu, Nov 2, 2023 at 2:50âPM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 02, 2023 at 12:40:03PM +0800, Jason Wang wrote:
> > On Wed, Nov 1, 2023 at 1:37âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Nov 01, 2023 at 12:16:23PM +0800, Jason Wang wrote:
> > > > On Sat, Oct 28, 2023 at 10:36âAM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > å 2023/10/27 äå3:39, Michael S. Tsirkin åé:
> > > > > > On Thu, Oct 19, 2023 at 02:17:20PM +0800, Heng Qi wrote:
> > > > > >> virtio-net works in a virtualized system and is somewhat different from
> > > > > >> physical nics. One of the differences is that to save virtio device
> > > > > >> resources, rx may receive packets with partial checksum. However, XDP may
> > > > > >> cause partially checksummed packets to be dropped. So XDP loading conflicts
> > > > > >> with the feature VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>
> > > > > >> This patch lets the device to supply fully checksummed packets to the driver.
> > > > > >> Then XDP can coexist with VIRTIO_NET_F_GUEST_CSUM to enjoy the benefits of
> > > > > >> device verification checksum.
> > > > > >>
> > > > > >> In addition, implementation of some performant devices do not generate
> > > > > >> partially checksummed packets, but the standard driver still need to clear
> > > > > >> VIRTIO_NET_F_GUEST_CSUM when loading XDP. If these devices enable the
> > > > > >> full checksum offloading, then the driver can load XDP without clearing
> > > > > >> VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>
> > > > > >> A new feature bit VIRTIO_NET_F_GUEST_FULL_CSUM is added to solve the above
> > > > > >> situation, which provides the driver with configurable receive full checksum
> > > > > >> offload. If the offload is enabled, then the device must supply fully
> > > > > >> checksummed packets to the driver.
> > > > > >>
> > > > > >> Use case example:
> > > > > >> If VIRTIO_NET_F_GUEST_FULL_CSUM is negotiated and receive full checksum
> > > > > >> offload is enabled, after XDP processes a packet with full checksum, the
> > > > > >> VIRTIO_NET_HDR_F_DATA_VALID bit is still retained, resulting in the stack
> > > > > >> not needing to validate the checksum again. This is useful for guests:
> > > > > >>    1. Bring the driver advantages such as cpu savings.
> > > > > >>    2. For devices that do not generate partially checksummed packets themselves,
> > > > > >>       XDP can be loaded in the driver without modifying the hardware behavior.
> > > > > >>
> > > > > >> Several solutions have been discussed in the previous proposal[1].
> > > > > >> After historical discussion, we have tried the method proposed by Jason[2],
> > > > > >> but some complex scenarios and challenges are difficult to deal with.
> > > > > >> We now return to the method suggested in [1].
> > > > > >>
> > > > > >> [1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00291.html
> > > > > >> [2] https://lore.kernel.org/all/20230628030506.2213-1-hengqi@linux.alibaba.com/
> > > > > >>
> > > > > >> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > >> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > >> ---
> > > > > >> v1->v2:
> > > > > >>      1. Modify full checksum functionality as a configurable offload
> > > > > >>         that is initially turned off. @Jason
> > > > > >>
> > > > > >>   device-types/net/description.tex | 54 ++++++++++++++++++++++++++++----
> > > > > >>   1 file changed, 48 insertions(+), 6 deletions(-)
> > > > > >>
> > > > > >> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> > > > > >> index 76585b0..3c34f27 100644
> > > > > >> --- a/device-types/net/description.tex
> > > > > >> +++ b/device-types/net/description.tex
> > > > > >> @@ -88,6 +88,8 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
> > > > > >>   \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > > > >>       channel.
> > > > > >>
> > > > > >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM (50)] Driver handles packets with full checksum.
> > > > > >> +
> > > > > >>   \item[VIRTIO_NET_F_HASH_TUNNEL(51)] Device supports inner header hash for encapsulated packets.
> > > > > >>
> > > > > >>   \item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports virtqueue notification coalescing.
> > > > > >> @@ -133,6 +135,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
> > > > > >>   \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>   \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >>   \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >> +\item[VIRTIO_NET_F_GUEST_FULL_CSUM] Requires VIRTIO_NET_F_GUEST_CSUM and VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > > > > >>
> > > > > >>   \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
> > > > > >>   \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
> > > > > > What about all of these:
> > > > > >
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > > device-types/net/description.tex:\item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> > > > > >
> > > > > >
> > > > > >
> > > > > > can TSO/UFO/USO work with VIRTIO_NET_F_GUEST_FULL_CSUM as opposed to VIRTIO_NET_F_GUEST_CSUM?
> > > > >
> > > > > Both GUEST_FULL_CSUM and GUEST_CSUM can work with GUEST_TSO/USO/UFO.
> > > >
> > > > Yes. For software devices I guess it will have a lot of performance
> > > > penalty. So it should be disabled by default anyhow. The idea is to
> > > > delay the csum as late as possible.
> > >
> > > But for hardware it's actually better.
> >
> > I can't think of a case where it might be better than XDP.
>
> Of course CHECKSUM_COMPLETE is better than CHECKSUM_PARTIAL if you can
> support it: you don't even need to look at csum_start + csum_offset.

I may miss something here, if the packet was delivered to the
userspace then they don't care.

> And the hardware doesn't need to parse L3/L4 headers to implement
> CHECKSUM_COMPLETE.

Hardware may choose to coalesce packets.

>
>
> > Most userspace doesn't care about the checksum though.
> >
> > > Maybe we need a flag
> > > to say which offloads are expensive?
> > >
> >
> > That exposes some device details which seem not good (e.g we may want
> > to do migration among hardware and software).
> >
> > Thanks
>
> If you do then things will be less well tuned on one of the migration
> ends but then that is by design, isn't it?

Ok, so I'm fine to enable it by default.

Thanks

>
> --
> MST
>