[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] [PATCH v2] virtio-net: support distinguishing between partial and full checksum
On Wed, Nov 1, 2023 at 12:59âPM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>
>
> å 2023/11/1 äå12:16, Jason Wang åé:
> > 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.
>
> Yes. I totally agree.
>
> >
> >> Their important difference is that if GUEST_CSUM is negotiated, the
> >> driver can handle partial checksum.
> >>
> >>>
> >>>> @@ -390,6 +393,13 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
> >>>> \ref{sec:Device Types / Network Device / Device Operation /
> >>>> Processing of Incoming Packets}~\nameref{sec:Device Types /
> >>>> Network Device / Device Operation / Processing of Incoming Packets} below.
> >>>> +
> >>>> +\item The VIRTIO_NET_F_GUEST_FULL_CSUM feature indicates that the driver handles
> >>>> + packets with full checksum and does not handle packets with partial checksum,
> >>> So we need to change definition of VIRTIO_NET_F_GUEST_CSUM then.
> >>>
> >>>
> >>> Also this is not exactly right. As defined driver must be able to handle
> >>> partial checksum too.
> >>>
> >>>
> >>> How about this:
> >>>
> >>> - change definition above to just "Driver handles packets with full checksum."
> >>>
> >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is set but VIRTIO_NET_F_GUEST_CSUM is
> >>> clear driver requires full checksum
> >>>
> >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM is clear but VIRTIO_NET_F_GUEST_CSUM is
> >>> set driver supports partial checksum
> >>>
> >>> - if VIRTIO_NET_F_GUEST_FULL_CSUM and VIRTIO_NET_F_GUEST_CSUM are
> >>> set then the behavior is as you describe: VIRTIO_NET_F_GUEST_CSUM
> >>> takes preference, but you can disable it with VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>> if that is supported.
> >> Jason wanted this feature to be enabled only when XDP is loading,
> >> and this is the context in which this patch was proposed.
> >>
> >> How do you pay attention to this?
> > I don't see any conflict, or anything I miss?
>
> Yes, our request was met.
>
> If GUEST_FULL_CSUM and GUEST_CSUM are independent,
> that is, GUEST_FULL_CSUM can be successfully validated without GUEST_CSUM.
> Then we need to re-describe most of the existing behavior of GUEST_CSUM
> for FULL_CSUM
> in the spec, this part is overlapping. Moreover, the relationship
> between FULL_CSUM
> and GUEST_CSUM also needs to be processed in the full text.
>
> So I think it seems clearer to constrain the behavior of GUEST_CSUM by
> treating FULL_CSUM as a subset of GUEST_CSUM.
This seems to work.
Thanks
>
> For example we don't need to make the following changes:
> \item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
> \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM or
> VIRTIO_NET_F_GUEST_FULL_CSUM.
>
> Thanks!
>
> >
> > Thanks
>
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]