| Message ID | 20230512152607.992209-4-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | new kfunc XDP hints and ice implementation | expand |
From: Larysa Zaremba <larysa.zaremba@intel.com> Date: Fri, 12 May 2023 17:25:55 +0200 > Previously, we only needed RX checksum flags in skb path, > hence all related code was written with skb in mind. > But with the addition of XDP hints via kfuncs to the ice driver, > the same logic will be needed in .xmo_() callbacks. > > Put generic process of determining checksum status into > a separate function. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 71 ++++++++++++------- > 1 file changed, 46 insertions(+), 25 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > index 1aab79dc8915..6a4fd3f3fc0a 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -104,17 +104,17 @@ ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring, > } > > /** > - * ice_rx_csum - Indicate in skb if checksum is good > - * @ring: the ring we care about > - * @skb: skb currently being received and modified > + * ice_rx_csum_checked - Indicates, whether hardware has checked the checksum %CHECKSUM_UNNECESSARY means that the csum is correct / frame is not damaged. So "checked" is not enough I'd say, it's "verified" at least. OTOH that's too long already, I'd go with classic "csum_ok" :D > * @rx_desc: the receive descriptor > * @ptype: the packet type decoded by hardware > + * @csum_lvl_dst: address to put checksum level into > + * @ring: ring for error stats, can be NULL > * > - * skb->protocol must be set before this function is called > + * Returns true, if hardware has checked the checksum. > */ > -static void > -ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, > - union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > +static bool > +ice_rx_csum_checked(union ice_32b_rx_flex_desc *rx_desc, u16 ptype, (also const, but I guess you'll do that either way after the previous mails) > + u8 *csum_lvl_dst, struct ice_rx_ring *ring) > { > struct ice_rx_ptype_decoded decoded; > u16 rx_status0, rx_status1; [...] > +/** > + * ice_rx_csum_into_skb - Indicate in skb if checksum is good > + * @ring: the ring we care about > + * @skb: skb currently being received and modified > + * @rx_desc: the receive descriptor > + * @ptype: the packet type decoded by hardware > + */ > +static void > +ice_rx_csum_into_skb(struct ice_rx_ring *ring, struct sk_buff *skb, > + union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > +{ > + u8 csum_level = 0; I'm not a fan of variables shorter than u32 on the stack. And since it gets passed by a reference, I'm not sure the compiler will inline it =\ > + > + /* Start with CHECKSUM_NONE and by default csum_level = 0 */ > + skb->ip_summed = CHECKSUM_NONE; > + skb_checksum_none_assert(skb); Can we also remove this? Neither of these makes sense. ::ip_summed is always zeroed after the memset() in __build_skb_around() (somewhere there), while the assertion checks for `skb->ip_summed == CHECKSUM_NONE`, i.e. it's *always* true here (set and check :D). It's some ancient pathetic rituals copied over and over again from e100 centuries or so... ...and BTW the comment is misleading, because the code doesn't zero ::csum_level as they claim :D > + > + /* check if Rx checksum is enabled */ > + if (!(ring->netdev->features & NETIF_F_RXCSUM)) > + return; > + > + if (!ice_rx_csum_checked(rx_desc, ptype, &csum_level, ring)) > + return; > + > + skb->ip_summed = CHECKSUM_UNNECESSARY; > + skb->csum_level = csum_level; Since csum_level is useless when ip_summed is set to NONE, what do you think about making the function return -1, 0, or 1 without writing anything by reference? int csum_level; csum_level = ice_rx_csum_ok(rx_desc, ptype, ring); if (csum_level < 0) return; skb->ip_summed = CHECKSUM_UNNECESSARY; skb->csum_level = csum_level; I'm not saying it's better (might be a bit at codegen), just proposing. > } > > /** > @@ -232,7 +253,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring, > /* modifies the skb - consumes the enet header */ > skb->protocol = eth_type_trans(skb, rx_ring->netdev); > > - ice_rx_csum(rx_ring, skb, rx_desc, ptype); > + ice_rx_csum_into_skb(rx_ring, skb, rx_desc, ptype); > > if (rx_ring->ptp_rx) > ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb); Thanks, Olek
On Mon, May 22, 2023 at 05:51:37PM +0200, Alexander Lobakin wrote: > From: Larysa Zaremba <larysa.zaremba@intel.com> > Date: Fri, 12 May 2023 17:25:55 +0200 > > > Previously, we only needed RX checksum flags in skb path, > > hence all related code was written with skb in mind. > > But with the addition of XDP hints via kfuncs to the ice driver, > > the same logic will be needed in .xmo_() callbacks. > > > > Put generic process of determining checksum status into > > a separate function. > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 71 ++++++++++++------- > > 1 file changed, 46 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > index 1aab79dc8915..6a4fd3f3fc0a 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > @@ -104,17 +104,17 @@ ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring, > > } > > > > /** > > - * ice_rx_csum - Indicate in skb if checksum is good > > - * @ring: the ring we care about > > - * @skb: skb currently being received and modified > > + * ice_rx_csum_checked - Indicates, whether hardware has checked the checksum > > %CHECKSUM_UNNECESSARY means that the csum is correct / frame is not > damaged. So "checked" is not enough I'd say, it's "verified" at least. > OTOH that's too long already, I'd go with classic "csum_ok" :D 'csum_ok' sounds good :) 'csum_valid' if want to be fancy > > > * @rx_desc: the receive descriptor > > * @ptype: the packet type decoded by hardware > > + * @csum_lvl_dst: address to put checksum level into > > + * @ring: ring for error stats, can be NULL > > * > > - * skb->protocol must be set before this function is called > > + * Returns true, if hardware has checked the checksum. > > */ > > -static void > > -ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, > > - union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > > +static bool > > +ice_rx_csum_checked(union ice_32b_rx_flex_desc *rx_desc, u16 ptype, > > (also const, but I guess you'll do that either way after the previous > mails) OK > > > + u8 *csum_lvl_dst, struct ice_rx_ring *ring) > > { > > struct ice_rx_ptype_decoded decoded; > > u16 rx_status0, rx_status1; > > [...] > > > +/** > > + * ice_rx_csum_into_skb - Indicate in skb if checksum is good > > + * @ring: the ring we care about > > + * @skb: skb currently being received and modified > > + * @rx_desc: the receive descriptor > > + * @ptype: the packet type decoded by hardware > > + */ > > +static void > > +ice_rx_csum_into_skb(struct ice_rx_ring *ring, struct sk_buff *skb, > > + union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > > +{ > > + u8 csum_level = 0; > > I'm not a fan of variables shorter than u32 on the stack. And since it > gets passed by a reference, I'm not sure the compiler will inline it =\ > > > + > > + /* Start with CHECKSUM_NONE and by default csum_level = 0 */ > > + skb->ip_summed = CHECKSUM_NONE; > > + skb_checksum_none_assert(skb); > > Can we also remove this? Neither of these makes sense. ::ip_summed is > always zeroed after the memset() in __build_skb_around() (somewhere > there), while the assertion checks for `skb->ip_summed == > CHECKSUM_NONE`, i.e. it's *always* true here (set and check :D). It's > some ancient pathetic rituals copied over and over again from e100 > centuries or so... Will fix. > > ...and BTW the comment is misleading, because the code doesn't zero > ::csum_level as they claim :D > > > + > > + /* check if Rx checksum is enabled */ > > + if (!(ring->netdev->features & NETIF_F_RXCSUM)) > > + return; > > + > > + if (!ice_rx_csum_checked(rx_desc, ptype, &csum_level, ring)) > > + return; > > + > > + skb->ip_summed = CHECKSUM_UNNECESSARY; > > + skb->csum_level = csum_level; > > Since csum_level is useless when ip_summed is set to NONE, what do you > think about making the function return -1, 0, or 1 without writing > anything by reference? > > int csum_level; > > csum_level = ice_rx_csum_ok(rx_desc, ptype, ring); > if (csum_level < 0) > return; > > skb->ip_summed = CHECKSUM_UNNECESSARY; > skb->csum_level = csum_level; > > I'm not saying it's better (might be a bit at codegen), just proposing. I think it's worth a try. > > > } > > > > /** > > @@ -232,7 +253,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring, > > /* modifies the skb - consumes the enet header */ > > skb->protocol = eth_type_trans(skb, rx_ring->netdev); > > > > - ice_rx_csum(rx_ring, skb, rx_desc, ptype); > > + ice_rx_csum_into_skb(rx_ring, skb, rx_desc, ptype); > > > > if (rx_ring->ptp_rx) > > ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb); > > Thanks, > Olek
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c index 1aab79dc8915..6a4fd3f3fc0a 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c @@ -104,17 +104,17 @@ ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring, } /** - * ice_rx_csum - Indicate in skb if checksum is good - * @ring: the ring we care about - * @skb: skb currently being received and modified + * ice_rx_csum_checked - Indicates, whether hardware has checked the checksum * @rx_desc: the receive descriptor * @ptype: the packet type decoded by hardware + * @csum_lvl_dst: address to put checksum level into + * @ring: ring for error stats, can be NULL * - * skb->protocol must be set before this function is called + * Returns true, if hardware has checked the checksum. */ -static void -ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, - union ice_32b_rx_flex_desc *rx_desc, u16 ptype) +static bool +ice_rx_csum_checked(union ice_32b_rx_flex_desc *rx_desc, u16 ptype, + u8 *csum_lvl_dst, struct ice_rx_ring *ring) { struct ice_rx_ptype_decoded decoded; u16 rx_status0, rx_status1; @@ -125,20 +125,12 @@ ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, decoded = ice_decode_rx_desc_ptype(ptype); - /* Start with CHECKSUM_NONE and by default csum_level = 0 */ - skb->ip_summed = CHECKSUM_NONE; - skb_checksum_none_assert(skb); - - /* check if Rx checksum is enabled */ - if (!(ring->netdev->features & NETIF_F_RXCSUM)) - return; - /* check if HW has decoded the packet and checksum */ if (!(rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_L3L4P_S))) - return; + return false; if (!(decoded.known && decoded.outer_ip)) - return; + return false; ipv4 = (decoded.outer_ip == ICE_RX_PTYPE_OUTER_IP) && (decoded.outer_ip_ver == ICE_RX_PTYPE_OUTER_IPV4); @@ -168,22 +160,51 @@ ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, * we are indicating we validated the inner checksum. */ if (decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT) - skb->csum_level = 1; + *csum_lvl_dst = 1; /* Only report checksum unnecessary for TCP, UDP, or SCTP */ switch (decoded.inner_prot) { case ICE_RX_PTYPE_INNER_PROT_TCP: case ICE_RX_PTYPE_INNER_PROT_UDP: case ICE_RX_PTYPE_INNER_PROT_SCTP: - skb->ip_summed = CHECKSUM_UNNECESSARY; - break; - default: - break; + return true; } - return; + + return false; checksum_fail: - ring->vsi->back->hw_csum_rx_error++; + if (ring) + ring->vsi->back->hw_csum_rx_error++; + + return false; +} + +/** + * ice_rx_csum_into_skb - Indicate in skb if checksum is good + * @ring: the ring we care about + * @skb: skb currently being received and modified + * @rx_desc: the receive descriptor + * @ptype: the packet type decoded by hardware + */ +static void +ice_rx_csum_into_skb(struct ice_rx_ring *ring, struct sk_buff *skb, + union ice_32b_rx_flex_desc *rx_desc, u16 ptype) +{ + u8 csum_level = 0; + + /* Start with CHECKSUM_NONE and by default csum_level = 0 */ + skb->ip_summed = CHECKSUM_NONE; + skb_checksum_none_assert(skb); + + /* check if Rx checksum is enabled */ + if (!(ring->netdev->features & NETIF_F_RXCSUM)) + return; + + if (!ice_rx_csum_checked(rx_desc, ptype, &csum_level, ring)) + return; + + skb->ip_summed = CHECKSUM_UNNECESSARY; + skb->csum_level = csum_level; } /** @@ -232,7 +253,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring, /* modifies the skb - consumes the enet header */ skb->protocol = eth_type_trans(skb, rx_ring->netdev); - ice_rx_csum(rx_ring, skb, rx_desc, ptype); + ice_rx_csum_into_skb(rx_ring, skb, rx_desc, ptype); if (rx_ring->ptp_rx) ice_ptp_rx_hwts_to_skb(rx_ring, rx_desc, skb);
Previously, we only needed RX checksum flags in skb path, hence all related code was written with skb in mind. But with the addition of XDP hints via kfuncs to the ice driver, the same logic will be needed in .xmo_() callbacks. Put generic process of determining checksum status into a separate function. Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 71 ++++++++++++------- 1 file changed, 46 insertions(+), 25 deletions(-)