| Message ID | 20230824192703.712881-4-larysa.zaremba@intel.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | BPF |
| Headers | show |
| Series | XDP metadata via kfuncs for ice + mlx5 | expand |
On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote: > 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. > > Now we cannot operate directly on skb, when deducing > checksum status, therefore introduce an intermediate enum for checksum > status. Fortunately, in ice, we have only 4 possibilities: checksum > validated at level 0, validated at level 1, no checksum, checksum error. > Use 3 bits for more convenient conversion. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ > 1 file changed, 69 insertions(+), 36 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > index b2f241b73934..8b155a502b3b 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, > skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > } > > +enum ice_rx_csum_status { > + ICE_RX_CSUM_LVL_0 = 0, > + ICE_RX_CSUM_LVL_1 = BIT(0), > + ICE_RX_CSUM_NONE = BIT(1), > + ICE_RX_CSUM_ERROR = BIT(2), > + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, > +}; > + > /** > - * 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_lvl - Get checksum level from status > + * @status: driver-specific checksum status > + */ > +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) > +{ > + return status & ICE_RX_CSUM_LVL_1; > +} > + > +/** > + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic > + * @status: driver-specific checksum status > + */ > +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) > +{ > + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; return !(status & ICE_RX_CSUM_NONE); ? > +} > + > +/** > + * ice_get_rx_csum_status - Deduce checksum status from descriptor > * @rx_desc: the receive descriptor > * @ptype: the packet type decoded by hardware > * > - * skb->protocol must be set before this function is called > + * Returns driver-specific checksum status > */ > -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 enum ice_rx_csum_status > +ice_get_rx_csum_status(const union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > { > struct ice_rx_ptype_decoded decoded; > u16 rx_status0, rx_status1; > @@ -124,20 +147,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 ICE_RX_CSUM_NONE; > > if (!(decoded.known && decoded.outer_ip)) > - return; > + return ICE_RX_CSUM_NONE; > > ipv4 = (decoded.outer_ip == ICE_RX_PTYPE_OUTER_IP) && > (decoded.outer_ip_ver == ICE_RX_PTYPE_OUTER_IPV4); > @@ -146,43 +161,61 @@ ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, > > if (ipv4 && (rx_status0 & (BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_IPE_S) | > BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_EIPE_S)))) > - goto checksum_fail; > + return ICE_RX_CSUM_FAIL; > > if (ipv6 && (rx_status0 & (BIT(ICE_RX_FLEX_DESC_STATUS0_IPV6EXADD_S)))) > - goto checksum_fail; > + return ICE_RX_CSUM_FAIL; > > /* check for L4 errors and handle packets that were not able to be > * checksummed due to arrival speed > */ > if (rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_L4E_S)) > - goto checksum_fail; > + return ICE_RX_CSUM_FAIL; > > /* check for outer UDP checksum error in tunneled packets */ > if ((rx_status1 & BIT(ICE_RX_FLEX_DESC_STATUS1_NAT_S)) && > (rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_EUDPE_S))) > - goto checksum_fail; > - > - /* If there is an outer header present that might contain a checksum > - * we need to bump the checksum level by 1 to reflect the fact that > - * we are indicating we validated the inner checksum. > - */ > - if (decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT) > - skb->csum_level = 1; > + return ICE_RX_CSUM_FAIL; > > /* 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; > + /* If there is an outer header present that might contain > + * a checksum we need to bump the checksum level by 1 to reflect > + * the fact that we have validated the inner checksum. > + */ > + return decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT ? > + ICE_RX_CSUM_LVL_1 : ICE_RX_CSUM_LVL_0; > } > - return; > > -checksum_fail: > - ring->vsi->back->hw_csum_rx_error++; > + return ICE_RX_CSUM_NONE; > +} > + > +/** > + * 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, > + const union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > +{ > + enum ice_rx_csum_status csum_status; > + > + /* check if Rx checksum is enabled */ > + if (!(ring->netdev->features & NETIF_F_RXCSUM)) > + return; > + > + csum_status = ice_get_rx_csum_status(rx_desc, ptype); > + if (csum_status & ICE_RX_CSUM_ERROR) > + ring->vsi->back->hw_csum_rx_error++; > + > + skb->ip_summed = ice_rx_csum_ip_summed(csum_status); > + skb->csum_level = ice_rx_csum_lvl(csum_status); > } > > /** > @@ -229,7 +262,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); > -- > 2.41.0 >
On Mon, Sep 04, 2023 at 05:02:40PM +0200, Maciej Fijalkowski wrote: > On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote: > > 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. > > > > Now we cannot operate directly on skb, when deducing > > checksum status, therefore introduce an intermediate enum for checksum > > status. Fortunately, in ice, we have only 4 possibilities: checksum > > validated at level 0, validated at level 1, no checksum, checksum error. > > Use 3 bits for more convenient conversion. > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ > > 1 file changed, 69 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > index b2f241b73934..8b155a502b3b 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, > > skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > } > > > > +enum ice_rx_csum_status { > > + ICE_RX_CSUM_LVL_0 = 0, > > + ICE_RX_CSUM_LVL_1 = BIT(0), > > + ICE_RX_CSUM_NONE = BIT(1), > > + ICE_RX_CSUM_ERROR = BIT(2), > > + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, > > +}; > > + > > /** > > - * 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_lvl - Get checksum level from status > > + * @status: driver-specific checksum status > > + */ > > +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) > > +{ > > + return status & ICE_RX_CSUM_LVL_1; > > +} > > + > > +/** > > + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic > > + * @status: driver-specific checksum status > > + */ > > +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) > > +{ > > + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > return !(status & ICE_RX_CSUM_NONE); > > ? status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; is immediately understandable and results in 3 asm operations (I have checked): result = status >> 1; result ^= 1; result &= 1; I do not think "!(status & ICE_RX_CSUM_NONE);" could produce less. > > > +} > > + > > +/** > > + * ice_get_rx_csum_status - Deduce checksum status from descriptor > > * @rx_desc: the receive descriptor > > * @ptype: the packet type decoded by hardware > > * > > - * skb->protocol must be set before this function is called > > + * Returns driver-specific checksum status > > */ > > -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 enum ice_rx_csum_status > > +ice_get_rx_csum_status(const union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > > { > > struct ice_rx_ptype_decoded decoded; > > u16 rx_status0, rx_status1; > > @@ -124,20 +147,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 ICE_RX_CSUM_NONE; > > > > if (!(decoded.known && decoded.outer_ip)) > > - return; > > + return ICE_RX_CSUM_NONE; > > > > ipv4 = (decoded.outer_ip == ICE_RX_PTYPE_OUTER_IP) && > > (decoded.outer_ip_ver == ICE_RX_PTYPE_OUTER_IPV4); > > @@ -146,43 +161,61 @@ ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, > > > > if (ipv4 && (rx_status0 & (BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_IPE_S) | > > BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_EIPE_S)))) > > - goto checksum_fail; > > + return ICE_RX_CSUM_FAIL; > > > > if (ipv6 && (rx_status0 & (BIT(ICE_RX_FLEX_DESC_STATUS0_IPV6EXADD_S)))) > > - goto checksum_fail; > > + return ICE_RX_CSUM_FAIL; > > > > /* check for L4 errors and handle packets that were not able to be > > * checksummed due to arrival speed > > */ > > if (rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_L4E_S)) > > - goto checksum_fail; > > + return ICE_RX_CSUM_FAIL; > > > > /* check for outer UDP checksum error in tunneled packets */ > > if ((rx_status1 & BIT(ICE_RX_FLEX_DESC_STATUS1_NAT_S)) && > > (rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_EUDPE_S))) > > - goto checksum_fail; > > - > > - /* If there is an outer header present that might contain a checksum > > - * we need to bump the checksum level by 1 to reflect the fact that > > - * we are indicating we validated the inner checksum. > > - */ > > - if (decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT) > > - skb->csum_level = 1; > > + return ICE_RX_CSUM_FAIL; > > > > /* 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; > > + /* If there is an outer header present that might contain > > + * a checksum we need to bump the checksum level by 1 to reflect > > + * the fact that we have validated the inner checksum. > > + */ > > + return decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT ? > > + ICE_RX_CSUM_LVL_1 : ICE_RX_CSUM_LVL_0; > > } > > - return; > > > > -checksum_fail: > > - ring->vsi->back->hw_csum_rx_error++; > > + return ICE_RX_CSUM_NONE; > > +} > > + > > +/** > > + * 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, > > + const union ice_32b_rx_flex_desc *rx_desc, u16 ptype) > > +{ > > + enum ice_rx_csum_status csum_status; > > + > > + /* check if Rx checksum is enabled */ > > + if (!(ring->netdev->features & NETIF_F_RXCSUM)) > > + return; > > + > > + csum_status = ice_get_rx_csum_status(rx_desc, ptype); > > + if (csum_status & ICE_RX_CSUM_ERROR) > > + ring->vsi->back->hw_csum_rx_error++; > > + > > + skb->ip_summed = ice_rx_csum_ip_summed(csum_status); > > + skb->csum_level = ice_rx_csum_lvl(csum_status); > > } > > > > /** > > @@ -229,7 +262,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); > > -- > > 2.41.0 > >
On Mon, Sep 04, 2023 at 08:01:06PM +0200, Larysa Zaremba wrote: > On Mon, Sep 04, 2023 at 05:02:40PM +0200, Maciej Fijalkowski wrote: > > On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote: > > > 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. > > > > > > Now we cannot operate directly on skb, when deducing > > > checksum status, therefore introduce an intermediate enum for checksum > > > status. Fortunately, in ice, we have only 4 possibilities: checksum > > > validated at level 0, validated at level 1, no checksum, checksum error. > > > Use 3 bits for more convenient conversion. > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > --- > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ > > > 1 file changed, 69 insertions(+), 36 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > index b2f241b73934..8b155a502b3b 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, > > > skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > > } > > > > > > +enum ice_rx_csum_status { > > > + ICE_RX_CSUM_LVL_0 = 0, > > > + ICE_RX_CSUM_LVL_1 = BIT(0), > > > + ICE_RX_CSUM_NONE = BIT(1), > > > + ICE_RX_CSUM_ERROR = BIT(2), > > > + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, > > > +}; > > > + > > > /** > > > - * 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_lvl - Get checksum level from status > > > + * @status: driver-specific checksum status nit: describe retval? > > > + */ > > > +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) > > > +{ > > > + return status & ICE_RX_CSUM_LVL_1; > > > +} > > > + > > > +/** > > > + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic > > > + * @status: driver-specific checksum status ditto > > > + */ > > > +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) > > > +{ > > > + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > return !(status & ICE_RX_CSUM_NONE); > > > > ? > > status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > is immediately understandable and results in 3 asm operations (I have checked): > > result = status >> 1; > result ^= 1; > result &= 1; > > I do not think "!(status & ICE_RX_CSUM_NONE);" could produce less. oh, nice. Just the fact that branch being added caught my eye. (...)
On Tue, Sep 05, 2023 at 05:37:27PM +0200, Maciej Fijalkowski wrote: > On Mon, Sep 04, 2023 at 08:01:06PM +0200, Larysa Zaremba wrote: > > On Mon, Sep 04, 2023 at 05:02:40PM +0200, Maciej Fijalkowski wrote: > > > On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote: > > > > 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. > > > > > > > > Now we cannot operate directly on skb, when deducing > > > > checksum status, therefore introduce an intermediate enum for checksum > > > > status. Fortunately, in ice, we have only 4 possibilities: checksum > > > > validated at level 0, validated at level 1, no checksum, checksum error. > > > > Use 3 bits for more convenient conversion. > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > --- > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ > > > > 1 file changed, 69 insertions(+), 36 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > index b2f241b73934..8b155a502b3b 100644 > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, > > > > skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > > > } > > > > > > > > +enum ice_rx_csum_status { > > > > + ICE_RX_CSUM_LVL_0 = 0, > > > > + ICE_RX_CSUM_LVL_1 = BIT(0), > > > > + ICE_RX_CSUM_NONE = BIT(1), > > > > + ICE_RX_CSUM_ERROR = BIT(2), > > > > + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, > > > > +}; > > > > + > > > > /** > > > > - * 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_lvl - Get checksum level from status > > > > + * @status: driver-specific checksum status > > nit: describe retval? > I think that kernel-doc is already too much for a one-liner. Also, checksum level is fully explained in sk_buff documentation. > > > > + */ > > > > +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) > > > > +{ > > > > + return status & ICE_RX_CSUM_LVL_1; > > > > +} > > > > + > > > > +/** > > > > + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic > > > > + * @status: driver-specific checksum status > > ditto Same as above. Moreover, there are only 2 possible return values that anyone can easily look up. Describing them here would only balloon the file length. > > > > > + */ > > > > +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) > > > > +{ > > > > + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > > > return !(status & ICE_RX_CSUM_NONE); > > > > > > ? > > > > status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > is immediately understandable and results in 3 asm operations (I have checked): > > > > result = status >> 1; > > result ^= 1; > > result &= 1; > > > > I do not think "!(status & ICE_RX_CSUM_NONE);" could produce less. > > oh, nice. Just the fact that branch being added caught my eye. > > (...)
On Tue, Sep 05, 2023 at 06:53:37PM +0200, Larysa Zaremba wrote: > On Tue, Sep 05, 2023 at 05:37:27PM +0200, Maciej Fijalkowski wrote: > > On Mon, Sep 04, 2023 at 08:01:06PM +0200, Larysa Zaremba wrote: > > > On Mon, Sep 04, 2023 at 05:02:40PM +0200, Maciej Fijalkowski wrote: > > > > On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote: > > > > > 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. > > > > > > > > > > Now we cannot operate directly on skb, when deducing > > > > > checksum status, therefore introduce an intermediate enum for checksum > > > > > status. Fortunately, in ice, we have only 4 possibilities: checksum > > > > > validated at level 0, validated at level 1, no checksum, checksum error. > > > > > Use 3 bits for more convenient conversion. > > > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > > --- > > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ > > > > > 1 file changed, 69 insertions(+), 36 deletions(-) > > > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > index b2f241b73934..8b155a502b3b 100644 > > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, > > > > > skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > > > > } > > > > > > > > > > +enum ice_rx_csum_status { > > > > > + ICE_RX_CSUM_LVL_0 = 0, > > > > > + ICE_RX_CSUM_LVL_1 = BIT(0), > > > > > + ICE_RX_CSUM_NONE = BIT(1), > > > > > + ICE_RX_CSUM_ERROR = BIT(2), > > > > > + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, > > > > > +}; > > > > > + > > > > > /** > > > > > - * 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_lvl - Get checksum level from status > > > > > + * @status: driver-specific checksum status > > > > nit: describe retval? > > > > I think that kernel-doc is already too much for a one-liner. > Also, checksum level is fully explained in sk_buff documentation. > > > > > > + */ > > > > > +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) > > > > > +{ > > > > > + return status & ICE_RX_CSUM_LVL_1; > > > > > +} > > > > > + > > > > > +/** > > > > > + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic > > > > > + * @status: driver-specific checksum status > > > > ditto > > Same as above. Moreover, there are only 2 possible return values that anyone can > easily look up. Describing them here would only balloon the file length. You really think 5 additional lines would balloon the file length? :D I am not sure what to say here. We have many pretty pointless kdoc retval descriptions like 'returns 0 on success, error otherwise' but to me this is following the guidelines from Documentation/doc-guide/kernel-doc.rst. If i generate kdoc I don't want to open up the source code to easily look up retvals. Just my 0.02$, not a thing that I'd like to keep on arguing on :) > > > > > > > > + */ > > > > > +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) > > > > > +{ > > > > > + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > > > > > return !(status & ICE_RX_CSUM_NONE); > > > > > > > > ? > > > > > > status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > > > is immediately understandable and results in 3 asm operations (I have checked): > > > > > > result = status >> 1; > > > result ^= 1; > > > result &= 1; > > > > > > I do not think "!(status & ICE_RX_CSUM_NONE);" could produce less. > > > > oh, nice. Just the fact that branch being added caught my eye. > > > > (...)
On Tue, Sep 05, 2023 at 07:44:32PM +0200, Maciej Fijalkowski wrote: > On Tue, Sep 05, 2023 at 06:53:37PM +0200, Larysa Zaremba wrote: > > On Tue, Sep 05, 2023 at 05:37:27PM +0200, Maciej Fijalkowski wrote: > > > On Mon, Sep 04, 2023 at 08:01:06PM +0200, Larysa Zaremba wrote: > > > > On Mon, Sep 04, 2023 at 05:02:40PM +0200, Maciej Fijalkowski wrote: > > > > > On Thu, Aug 24, 2023 at 09:26:42PM +0200, Larysa Zaremba wrote: > > > > > > 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. > > > > > > > > > > > > Now we cannot operate directly on skb, when deducing > > > > > > checksum status, therefore introduce an intermediate enum for checksum > > > > > > status. Fortunately, in ice, we have only 4 possibilities: checksum > > > > > > validated at level 0, validated at level 1, no checksum, checksum error. > > > > > > Use 3 bits for more convenient conversion. > > > > > > > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > > > > > --- > > > > > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ > > > > > > 1 file changed, 69 insertions(+), 36 deletions(-) > > > > > > > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > index b2f241b73934..8b155a502b3b 100644 > > > > > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > > > > > @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, > > > > > > skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > > > > > } > > > > > > > > > > > > +enum ice_rx_csum_status { > > > > > > + ICE_RX_CSUM_LVL_0 = 0, > > > > > > + ICE_RX_CSUM_LVL_1 = BIT(0), > > > > > > + ICE_RX_CSUM_NONE = BIT(1), > > > > > > + ICE_RX_CSUM_ERROR = BIT(2), > > > > > > + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, > > > > > > +}; > > > > > > + > > > > > > /** > > > > > > - * 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_lvl - Get checksum level from status > > > > > > + * @status: driver-specific checksum status > > > > > > nit: describe retval? > > > > > > > I think that kernel-doc is already too much for a one-liner. > > Also, checksum level is fully explained in sk_buff documentation. > > > > > > > > + */ > > > > > > +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) > > > > > > +{ > > > > > > + return status & ICE_RX_CSUM_LVL_1; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic > > > > > > + * @status: driver-specific checksum status > > > > > > ditto > > > > Same as above. Moreover, there are only 2 possible return values that anyone can > > easily look up. Describing them here would only balloon the file length. > > You really think 5 additional lines would balloon the file length? :D > > I am not sure what to say here. We have many pretty pointless kdoc retval > descriptions like 'returns 0 on success, error otherwise' but to me this > is following the guidelines from Documentation/doc-guide/kernel-doc.rst. > If i generate kdoc I don't want to open up the source code to easily look > up retvals. > > Just my 0.02$, not a thing that I'd like to keep on arguing on :) > I have consulted with the team and we came to a conclusion that maybe removing kernel-doc for these functions would be the best solution. In ice, functions in source files are always documented, but I do not think this rule is set in stone. Sorry, if I was being rude, I had been traumatized by documentaion requirements at my previous job (automotive) :D > > > > > > > > > > > + */ > > > > > > +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) > > > > > > +{ > > > > > > + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > > > > > > > return !(status & ICE_RX_CSUM_NONE); > > > > > > > > > > ? > > > > > > > > status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; > > > > > > > > is immediately understandable and results in 3 asm operations (I have checked): > > > > > > > > result = status >> 1; > > > > result ^= 1; > > > > result &= 1; > > > > > > > > I do not think "!(status & ICE_RX_CSUM_NONE);" could produce less. > > > > > > oh, nice. Just the fact that branch being added caught my eye. > > > > > > (...)
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c index b2f241b73934..8b155a502b3b 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c @@ -102,18 +102,41 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring, skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); } +enum ice_rx_csum_status { + ICE_RX_CSUM_LVL_0 = 0, + ICE_RX_CSUM_LVL_1 = BIT(0), + ICE_RX_CSUM_NONE = BIT(1), + ICE_RX_CSUM_ERROR = BIT(2), + ICE_RX_CSUM_FAIL = ICE_RX_CSUM_NONE | ICE_RX_CSUM_ERROR, +}; + /** - * 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_lvl - Get checksum level from status + * @status: driver-specific checksum status + */ +static u8 ice_rx_csum_lvl(enum ice_rx_csum_status status) +{ + return status & ICE_RX_CSUM_LVL_1; +} + +/** + * ice_rx_csum_ip_summed - Checksum status from driver-specific to generic + * @status: driver-specific checksum status + */ +static u8 ice_rx_csum_ip_summed(enum ice_rx_csum_status status) +{ + return status & ICE_RX_CSUM_NONE ? CHECKSUM_NONE : CHECKSUM_UNNECESSARY; +} + +/** + * ice_get_rx_csum_status - Deduce checksum status from descriptor * @rx_desc: the receive descriptor * @ptype: the packet type decoded by hardware * - * skb->protocol must be set before this function is called + * Returns driver-specific checksum status */ -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 enum ice_rx_csum_status +ice_get_rx_csum_status(const union ice_32b_rx_flex_desc *rx_desc, u16 ptype) { struct ice_rx_ptype_decoded decoded; u16 rx_status0, rx_status1; @@ -124,20 +147,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 ICE_RX_CSUM_NONE; if (!(decoded.known && decoded.outer_ip)) - return; + return ICE_RX_CSUM_NONE; ipv4 = (decoded.outer_ip == ICE_RX_PTYPE_OUTER_IP) && (decoded.outer_ip_ver == ICE_RX_PTYPE_OUTER_IPV4); @@ -146,43 +161,61 @@ ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff *skb, if (ipv4 && (rx_status0 & (BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_IPE_S) | BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_EIPE_S)))) - goto checksum_fail; + return ICE_RX_CSUM_FAIL; if (ipv6 && (rx_status0 & (BIT(ICE_RX_FLEX_DESC_STATUS0_IPV6EXADD_S)))) - goto checksum_fail; + return ICE_RX_CSUM_FAIL; /* check for L4 errors and handle packets that were not able to be * checksummed due to arrival speed */ if (rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_L4E_S)) - goto checksum_fail; + return ICE_RX_CSUM_FAIL; /* check for outer UDP checksum error in tunneled packets */ if ((rx_status1 & BIT(ICE_RX_FLEX_DESC_STATUS1_NAT_S)) && (rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_XSUM_EUDPE_S))) - goto checksum_fail; - - /* If there is an outer header present that might contain a checksum - * we need to bump the checksum level by 1 to reflect the fact that - * we are indicating we validated the inner checksum. - */ - if (decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT) - skb->csum_level = 1; + return ICE_RX_CSUM_FAIL; /* 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; + /* If there is an outer header present that might contain + * a checksum we need to bump the checksum level by 1 to reflect + * the fact that we have validated the inner checksum. + */ + return decoded.tunnel_type >= ICE_RX_PTYPE_TUNNEL_IP_GRENAT ? + ICE_RX_CSUM_LVL_1 : ICE_RX_CSUM_LVL_0; } - return; -checksum_fail: - ring->vsi->back->hw_csum_rx_error++; + return ICE_RX_CSUM_NONE; +} + +/** + * 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, + const union ice_32b_rx_flex_desc *rx_desc, u16 ptype) +{ + enum ice_rx_csum_status csum_status; + + /* check if Rx checksum is enabled */ + if (!(ring->netdev->features & NETIF_F_RXCSUM)) + return; + + csum_status = ice_get_rx_csum_status(rx_desc, ptype); + if (csum_status & ICE_RX_CSUM_ERROR) + ring->vsi->back->hw_csum_rx_error++; + + skb->ip_summed = ice_rx_csum_ip_summed(csum_status); + skb->csum_level = ice_rx_csum_lvl(csum_status); } /** @@ -229,7 +262,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. Now we cannot operate directly on skb, when deducing checksum status, therefore introduce an intermediate enum for checksum status. Fortunately, in ice, we have only 4 possibilities: checksum validated at level 0, validated at level 1, no checksum, checksum error. Use 3 bits for more convenient conversion. Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 105 ++++++++++++------ 1 file changed, 69 insertions(+), 36 deletions(-)