add partial rx hw csum offload support for XDP

Introduce flag field in xdp_buff and xdp_frame data structure in order to report xdp_buffer metadata. For the moment just hw checksum hints are defined but flags field will be reused for xdp multi-buffer For the moment just CHECKSUM_UNNECESSARY is supported. CHECKSUM_COMPLETE will need to set csum value in metada space.

Signed-off-by David Ahern <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

include/net/xdp.h +36 -1

index 5533f0ab2afc..e81ac505752b 100644
@@ -66,6 +66,13 @@ struct xdp_txq_info {
        struct net_device *dev;
 };
 
+/* xdp metadata bitmask */
+#define XDP_CSUM_MASK          GENMASK(1, 0)
+enum xdp_flags {
+       XDP_CSUM_UNNECESSARY    = BIT(0),
+       XDP_CSUM_COMPLETE       = BIT(1),
+};
+
 struct xdp_buff {
        void *data;
        void *data_end;

@@ -74,6 +81,7 @@ struct xdp_buff {
        struct xdp_rxq_info *rxq;
        struct xdp_txq_info *txq;
        u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+       u16 flags; /* xdp_flags */
 };
 
 static __always_inline void

@@ -81,6 +89,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
        xdp->frame_sz = frame_sz;
        xdp->rxq = rxq;
+       xdp->flags = 0;
 }
 
 static __always_inline void

@@ -95,6 +104,18 @@ xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
        xdp->data_meta = meta_valid ? data : data + 1;
 }
 
+static __always_inline void
+xdp_buff_get_csum(struct xdp_buff *xdp, struct sk_buff *skb)
+{
+       switch (xdp->flags & XDP_CSUM_MASK) {
+       case XDP_CSUM_UNNECESSARY:
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               break;
+       default:
+               break;
+       }
+}
+
 /* Reserve memory area at end-of data area.
  *
  * This macro reserves tailroom in the XDP buffer by limiting the

@@ -122,8 +143,21 @@ struct xdp_frame {
         */
        struct xdp_mem_info mem;
        struct net_device *dev_rx; /* used by cpumap */
+       u16 flags; /* xdp_flags */
 };
 
+static __always_inline void
+xdp_frame_get_csum(struct xdp_frame *xdpf, struct sk_buff *skb)
+{
+       switch (xdpf->flags & XDP_CSUM_MASK) {
+       case XDP_CSUM_UNNECESSARY:
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               break;
+       default:
+               break;
+       }
+}
+
 #define XDP_BULK_QUEUE_SIZE    16
 struct xdp_frame_bulk {
        int count;

@@ -180,6 +214,7 @@ void xdp_convert_frame_to_buff(struct xdp_frame *frame, struct xdp_buff *xdp)
        xdp->data_end = frame->data + frame->len;
        xdp->data_meta = frame->data - frame->metasize;
        xdp->frame_sz = frame->frame_sz;
+       xdp->flags = frame->flags;
 }
 
 static inline

@@ -206,6 +241,7 @@ int xdp_update_frame_from_buff(struct xdp_buff *xdp,
        xdp_frame->headroom = headroom - sizeof(*xdp_frame);
        xdp_frame->metasize = metasize;
        xdp_frame->frame_sz = xdp->frame_sz;
+       xdp_frame->flags = xdp->flags;
 
        return 0;
 }
--

2.31.1

Quoted 7 lines · Lorenzo Bianconi

Introduce flag field in xdp_buff and xdp_frame data structure in order to report xdp_buffer metadata. For the moment just hw checksum hints are defined but flags field will be reused for xdp multi-buffer For the moment just CHECKSUM_UNNECESSARY is supported. CHECKSUM_COMPLETE will need to set csum value in metada space.

Lorenzo,

This isn't sufficient for the checksum-unnecessary interface, we'd also need ability to set csum_level for cases the device validated more than one checksum.

IMO, we shouldn't support CHECKSUM_UNNECESSARY for new uses like this. For years now, the Linux community has been pleading with vendors to provide CHECKSUM_COMPLETE which is far more useful and robust than CHECSUM_UNNECESSARY, and yet some still haven't got with the program even though we see more and more instances where CHECKSUM_UNNECESSARY doesn't even work at all (e.g. cases with SRv6, new encaps device doesn't understand). I believe it's time to take a stand! :-)

Quoted 6 lines · Lorenzo Bianconi

Introduce flag field in xdp_buff and xdp_frame data structure in order to report xdp_buffer metadata. For the moment just hw checksum hints are defined but flags field will be reused for xdp multi-buffer For the moment just CHECKSUM_UNNECESSARY is supported. CHECKSUM_COMPLETE will need to set csum value in metada space.

Lorenzo,

This isn't sufficient for the checksum-unnecessary interface, we'd also need ability to set csum_level for cases the device validated more than one checksum.

IMO, we shouldn't support CHECKSUM_UNNECESSARY for new uses like this. For years now, the Linux community has been pleading with vendors to provide CHECKSUM_COMPLETE which is far more useful and robust than CHECSUM_UNNECESSARY, and yet some still haven't got with the program even though we see more and more instances where CHECKSUM_UNNECESSARY doesn't even work at all (e.g. cases with SRv6, new encaps device doesn't understand). I believe it's time to take a stand! :-)

Quoted 13 lines · Lorenzo Bianconi

Quoted 6 lines

Introduce flag field in xdp_buff and xdp_frame data structure in order to report xdp_buffer metadata. For the moment just hw checksum hints are defined but flags field will be reused for xdp multi-buffer For the moment just CHECKSUM_UNNECESSARY is supported. CHECKSUM_COMPLETE will need to set csum value in metada space.

Lorenzo,

This isn't sufficient for the checksum-unnecessary interface, we'd also need ability to set csum_level for cases the device validated more than one checksum.

Quoted 7 lines

IMO, we shouldn't support CHECKSUM_UNNECESSARY for new uses like this. For years now, the Linux community has been pleading with vendors to provide CHECKSUM_COMPLETE which is far more useful and robust than CHECSUM_UNNECESSARY, and yet some still haven't got with the program even though we see more and more instances where CHECKSUM_UNNECESSARY doesn't even work at all (e.g. cases with SRv6, new encaps device doesn't understand). I believe it's time to take a stand! :-)

I must agree. Not supporting CHECKSUM_COMPLETE seems like a step back.

Quoted 7 lines

Introduce flag field in xdp_buff and xdp_frame data structure in order to report xdp_buffer metadata. For the moment just hw checksum hints are defined but flags field will be reused for xdp multi-buffer For the moment just CHECKSUM_UNNECESSARY is supported. CHECKSUM_COMPLETE will need to set csum value in metada space.

Lorenzo,

This isn't sufficient for the checksum-unnecessary interface, we'd also need ability to set csum_level for cases the device validated more than one checksum.

IMO, we shouldn't support CHECKSUM_UNNECESSARY for new uses like this. For years now, the Linux community has been pleading with vendors to provide CHECKSUM_COMPLETE which is far more useful and robust than CHECSUM_UNNECESSARY, and yet some still haven't got with the program even though we see more and more instances where CHECKSUM_UNNECESSARY doesn't even work at all (e.g. cases with SRv6, new encaps device doesn't understand). I believe it's time to take a stand! :-)

Tom

Quoted 100 lines

Signed-off-by David Ahern <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

include/net/xdp.h +36 -1

index 5533f0ab2afc..e81ac505752b 100644
@@ -66,6 +66,13 @@ struct xdp_txq_info {
        struct net_device *dev;
 };
 
+/* xdp metadata bitmask */
+#define XDP_CSUM_MASK          GENMASK(1, 0)
+enum xdp_flags {
+       XDP_CSUM_UNNECESSARY    = BIT(0),
+       XDP_CSUM_COMPLETE       = BIT(1),
+};
+
 struct xdp_buff {
        void *data;
        void *data_end;

@@ -74,6 +81,7 @@ struct xdp_buff {
        struct xdp_rxq_info *rxq;
        struct xdp_txq_info *txq;
        u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+       u16 flags; /* xdp_flags */
 };
 
 static __always_inline void

@@ -81,6 +89,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
        xdp->frame_sz = frame_sz;
        xdp->rxq = rxq;
+       xdp->flags = 0;
 }
 
 static __always_inline void

@@ -95,6 +104,18 @@ xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
        xdp->data_meta = meta_valid ? data : data + 1;
 }
 
+static __always_inline void
+xdp_buff_get_csum(struct xdp_buff *xdp, struct sk_buff *skb)
+{
+       switch (xdp->flags & XDP_CSUM_MASK) {
+       case XDP_CSUM_UNNECESSARY:
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               break;
+       default:
+               break;
+       }
+}
+
 /* Reserve memory area at end-of data area.
  *
  * This macro reserves tailroom in the XDP buffer by limiting the

@@ -122,8 +143,21 @@ struct xdp_frame {
         */
        struct xdp_mem_info mem;
        struct net_device *dev_rx; /* used by cpumap */
+       u16 flags; /* xdp_flags */
 };
 
+static __always_inline void
+xdp_frame_get_csum(struct xdp_frame *xdpf, struct sk_buff *skb)
+{
+       switch (xdpf->flags & XDP_CSUM_MASK) {
+       case XDP_CSUM_UNNECESSARY:
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               break;
+       default:
+               break;
+       }
+}
+
 #define XDP_BULK_QUEUE_SIZE    16
 struct xdp_frame_bulk {
        int count;

@@ -180,6 +214,7 @@ void xdp_convert_frame_to_buff(struct xdp_frame *frame, struct xdp_buff *xdp)
        xdp->data_end = frame->data + frame->len;
        xdp->data_meta = frame->data - frame->metasize;
        xdp->frame_sz = frame->frame_sz;
+       xdp->flags = frame->flags;
 }
 
 static inline

@@ -206,6 +241,7 @@ int xdp_update_frame_from_buff(struct xdp_buff *xdp,
        xdp_frame->headroom = headroom - sizeof(*xdp_frame);
        xdp_frame->metasize = metasize;
        xdp_frame->frame_sz = xdp->frame_sz;
+       xdp_frame->flags = xdp->flags;
 
        return 0;
 }
--

2.31.1

Quoted 9 lines · Lorenzo Bianconi

Quoted 7 lines

Introduce flag field in xdp_buff and xdp_frame data structure in order to report xdp_buffer metadata. For the moment just hw checksum hints are defined but flags field will be reused for xdp multi-buffer For the moment just CHECKSUM_UNNECESSARY is supported. CHECKSUM_COMPLETE will need to set csum value in metada space.

Lorenzo,

Hi Tom,

Quoted 1 line

Quoted 3 lines

This isn't sufficient for the checksum-unnecessary interface, we'd also need ability to set csum_level for cases the device validated more than one checksum.

ack, right. I guess we can put this info in xdp metadata or do you think we can add it in xdp_buff/xdp_frame as well?

Quoted 8 lines

IMO, we shouldn't support CHECKSUM_UNNECESSARY for new uses like this. For years now, the Linux community has been pleading with vendors to provide CHECKSUM_COMPLETE which is far more useful and robust than CHECSUM_UNNECESSARY, and yet some still haven't got with the program even though we see more and more instances where CHECKSUM_UNNECESSARY doesn't even work at all (e.g. cases with SRv6, new encaps device doesn't understand). I believe it's time to take a stand! :-)

I completely agree CHECKSUM_COMPLETE is more useful and robust than CHECSUM_UNNECESSARY and I want to add support for it as soon as we agree on the best way to do it. At the same time there are plenty of XDP NICs where this feature is quite useful since they support just CHECSUM_UNNECESSARY.

Regards,
Lorenzo

Quoted 103 lines

Tom

Quoted 100 lines

Signed-off-by David Ahern <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 include/net/xdp.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

include/net/xdp.h +36 -1

index 5533f0ab2afc..e81ac505752b 100644
@@ -66,6 +66,13 @@ struct xdp_txq_info {
        struct net_device *dev;
 };
 
+/* xdp metadata bitmask */
+#define XDP_CSUM_MASK          GENMASK(1, 0)
+enum xdp_flags {
+       XDP_CSUM_UNNECESSARY    = BIT(0),
+       XDP_CSUM_COMPLETE       = BIT(1),
+};
+
 struct xdp_buff {
        void *data;
        void *data_end;

@@ -74,6 +81,7 @@ struct xdp_buff {
        struct xdp_rxq_info *rxq;
        struct xdp_txq_info *txq;
        u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
+       u16 flags; /* xdp_flags */
 };
 
 static __always_inline void

@@ -81,6 +89,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
 {
        xdp->frame_sz = frame_sz;
        xdp->rxq = rxq;
+       xdp->flags = 0;
 }
 
 static __always_inline void

@@ -95,6 +104,18 @@ xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
        xdp->data_meta = meta_valid ? data : data + 1;
 }
 
+static __always_inline void
+xdp_buff_get_csum(struct xdp_buff *xdp, struct sk_buff *skb)
+{
+       switch (xdp->flags & XDP_CSUM_MASK) {
+       case XDP_CSUM_UNNECESSARY:
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               break;
+       default:
+               break;
+       }
+}
+
 /* Reserve memory area at end-of data area.
  *
  * This macro reserves tailroom in the XDP buffer by limiting the

@@ -122,8 +143,21 @@ struct xdp_frame {
         */
        struct xdp_mem_info mem;
        struct net_device *dev_rx; /* used by cpumap */
+       u16 flags; /* xdp_flags */
 };
 
+static __always_inline void
+xdp_frame_get_csum(struct xdp_frame *xdpf, struct sk_buff *skb)
+{
+       switch (xdpf->flags & XDP_CSUM_MASK) {
+       case XDP_CSUM_UNNECESSARY:
+               skb->ip_summed = CHECKSUM_UNNECESSARY;
+               break;
+       default:
+               break;
+       }
+}
+
 #define XDP_BULK_QUEUE_SIZE    16
 struct xdp_frame_bulk {
        int count;

@@ -180,6 +214,7 @@ void xdp_convert_frame_to_buff(struct xdp_frame *frame, struct xdp_buff *xdp)
        xdp->data_end = frame->data + frame->len;
        xdp->data_meta = frame->data - frame->metasize;
        xdp->frame_sz = frame->frame_sz;
+       xdp->flags = frame->flags;
 }
 
 static inline

@@ -206,6 +241,7 @@ int xdp_update_frame_from_buff(struct xdp_buff *xdp,
        xdp_frame->headroom = headroom - sizeof(*xdp_frame);
        xdp_frame->metasize = metasize;
        xdp_frame->frame_sz = xdp->frame_sz;
+       xdp_frame->flags = xdp->flags;
 
        return 0;
 }
--

2.31.1

Enable xdp rx checksum offload support for CHECKSUM_UNNECESSARY use-case. Introduce flag field in xdp_buff/xdp_frame in order to save the checksum result from the NIC and have a fast access to it performing XDP_REDIRECT. CHECKSUM_COMPLETE is not supported yet since it will require adding the csum result to the xdp_metadata area.
Moreover flag field will be reused for xdp multi-buff support. This series has been tested generating UDP traffic with pktgen and performing a xdp_redirect from an ixgbe device to a remote CPUMAP entry. PPS results show a negligible penalty respect to the baseline where the UDP checksum has been disabled. More info about the test can be found here [0].

[0] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_frame01_checksum.org