[ovs-dev,v4,1/2] dp-packet: Set checksum flags during software TSO.

When OVS needs to fallback on the software TSO implementation to segment
a packet, it currently doesn't guarantee that IP and TCP checksum
offload flags are set. However, it is possible that these is required.
This is true in the case of dp_netdev_upcall(), which clears these
flags.

This patch explicitly sets the appropriate flags when the segmentation
flag is removed, to guarantee that packets always end up with correct
checksums.

Signed-off-by: Mike Pattrick <mkp@redhat.com>

---

v3: Moved logic from ofproto-dpif-upcall to dp-packet
Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 lib/dp-packet.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

On Tue, Dec 19, 2023 at 09:47:18AM -0500, Mike Pattrick wrote:
> When OVS needs to fallback on the software TSO implementation to segment
> a packet, it currently doesn't guarantee that IP and TCP checksum
> offload flags are set. However, it is possible that these is required.
> This is true in the case of dp_netdev_upcall(), which clears these
> flags.
> 
> This patch explicitly sets the appropriate flags when the segmentation
> flag is removed, to guarantee that packets always end up with correct
> checksums.
> 
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

Acked-by: Simon Horman <horms@ovn.org>

On 12/19/23 15:47, Mike Pattrick wrote:
> When OVS needs to fallback on the software TSO implementation to segment
> a packet, it currently doesn't guarantee that IP and TCP checksum
> offload flags are set. However, it is possible that these is required.
> This is true in the case of dp_netdev_upcall(), which clears these
> flags.
> 
> This patch explicitly sets the appropriate flags when the segmentation
> flag is removed, to guarantee that packets always end up with correct
> checksums.
> 
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> 
> ---
> 
> v3: Moved logic from ofproto-dpif-upcall to dp-packet
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---
>  lib/dp-packet.h | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 11aa00723..f91b5e3fb 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -1131,11 +1131,23 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
>      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
>  }
>  
> -/* Resets TCP Segmentation flag in packet 'p'. */
> +/* Resets TCP Segmentation in packet 'p' and adjust flags to indicate
> + * L3 and L4 checksumming is now required. */
>  static inline void
>  dp_packet_hwol_reset_tcp_seg(struct dp_packet *p)
>  {
> -    *dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_TX_TCP_SEG;
> +    uint64_t ol_flags = *dp_packet_ol_flags_ptr(p)
> +                        | DP_PACKET_OL_TX_TCP_CKSUM;
> +
> +    ol_flags = ol_flags & ~(DP_PACKET_OL_TX_TCP_SEG
> +                            | DP_PACKET_OL_RX_L4_CKSUM_MASK
> +                            | DP_PACKET_OL_RX_IP_CKSUM_GOOD);

This is a little unclear why we clear all L4 checksum flags,
but we do not clear IP_CKSUM_BAD.  Is there a reason for that?
If so, we should have a comment here explaining the situation
as it is not obvious.

> +
> +    if (ol_flags & DP_PACKET_OL_TX_IPV4) {
> +         ol_flags |= DP_PACKET_OL_TX_IP_CKSUM;

Nit: Over-indented.  One space too far.

> +    }
> +
> +    *dp_packet_ol_flags_ptr(p) = ol_flags;
>  }
>  
>  /* Returns 'true' if the IP header has good integrity and the

On Sat, Jan 13, 2024 at 4:21 PM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 12/19/23 15:47, Mike Pattrick wrote:
> > When OVS needs to fallback on the software TSO implementation to segment
> > a packet, it currently doesn't guarantee that IP and TCP checksum
> > offload flags are set. However, it is possible that these is required.
> > This is true in the case of dp_netdev_upcall(), which clears these
> > flags.
> >
> > This patch explicitly sets the appropriate flags when the segmentation
> > flag is removed, to guarantee that packets always end up with correct
> > checksums.
> >
> > Signed-off-by: Mike Pattrick <mkp@redhat.com>
> >
> > ---
> >
> > v3: Moved logic from ofproto-dpif-upcall to dp-packet
> > Signed-off-by: Mike Pattrick <mkp@redhat.com>
> > ---
> >  lib/dp-packet.h | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> > index 11aa00723..f91b5e3fb 100644
> > --- a/lib/dp-packet.h
> > +++ b/lib/dp-packet.h
> > @@ -1131,11 +1131,23 @@ dp_packet_hwol_set_tcp_seg(struct dp_packet *b)
> >      *dp_packet_ol_flags_ptr(b) |= DP_PACKET_OL_TX_TCP_SEG;
> >  }
> >
> > -/* Resets TCP Segmentation flag in packet 'p'. */
> > +/* Resets TCP Segmentation in packet 'p' and adjust flags to indicate
> > + * L3 and L4 checksumming is now required. */
> >  static inline void
> >  dp_packet_hwol_reset_tcp_seg(struct dp_packet *p)
> >  {
> > -    *dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_TX_TCP_SEG;
> > +    uint64_t ol_flags = *dp_packet_ol_flags_ptr(p)
> > +                        | DP_PACKET_OL_TX_TCP_CKSUM;
> > +
> > +    ol_flags = ol_flags & ~(DP_PACKET_OL_TX_TCP_SEG
> > +                            | DP_PACKET_OL_RX_L4_CKSUM_MASK
> > +                            | DP_PACKET_OL_RX_IP_CKSUM_GOOD);
>
> This is a little unclear why we clear all L4 checksum flags,
> but we do not clear IP_CKSUM_BAD.  Is there a reason for that?
> If so, we should have a comment here explaining the situation
> as it is not obvious.

Removing BAD is unneeded here. I'll fix that in the next version.


Thanks,
Mike

>
> > +
> > +    if (ol_flags & DP_PACKET_OL_TX_IPV4) {
> > +         ol_flags |= DP_PACKET_OL_TX_IP_CKSUM;
>
> Nit: Over-indented.  One space too far.
>
> > +    }
> > +
> > +    *dp_packet_ol_flags_ptr(p) = ol_flags;
> >  }
> >
> >  /* Returns 'true' if the IP header has good integrity and the
>