| Message ID | 20230512152607.992209-2-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:53 +0200 > Previously, we only needed RX hash 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. > > Separate generic process of reading RX hash from a descriptor > into a separate function. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 38 +++++++++++++------ > 1 file changed, 27 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > index c8322fb6f2b3..fc67bbf600af 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -63,28 +63,44 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) > } > > /** > - * ice_rx_hash - set the hash value in the skb > + * ice_copy_rx_hash_from_desc - copy hash value from descriptor to address > + * @rx_desc: specific descriptor > + * @dst: address to copy hash value to > + * > + * Returns true, if valid hash has been copied into the destination address. > + */ > +static bool > +ice_copy_rx_hash_from_desc(union ice_32b_rx_flex_desc *rx_desc, u32 *dst) @rx_desc can be const. I'm also unsure about the naming. Why not name this one ice_rx_hash() and the one which sets it in skb ice_rx_hash_skb()? > +{ > + struct ice_32b_rx_flex_desc_nic *nic_mdid; Also const. I thought you'll pick most of my optimizations from the related commit :D > + > + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) > + return false; > + > + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > + *dst = le32_to_cpu(nic_mdid->rss_hash); > + return true; You can just return the hash. `hash == 0` means there's no hash, so it basically means `false`, while non-zero is `true`. > +} > + > +/** > + * ice_rx_hash_to_skb - set the hash value in the skb > * @rx_ring: descriptor ring > * @rx_desc: specific descriptor > * @skb: pointer to current skb > * @rx_ptype: the ptype value from the descriptor > */ > static void > -ice_rx_hash(struct ice_rx_ring *rx_ring, union ice_32b_rx_flex_desc *rx_desc, > - struct sk_buff *skb, u16 rx_ptype) > +ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring, > + union ice_32b_rx_flex_desc *rx_desc, > + struct sk_buff *skb, u16 rx_ptype) > { > - struct ice_32b_rx_flex_desc_nic *nic_mdid; > u32 hash; > > if (!(rx_ring->netdev->features & NETIF_F_RXHASH)) > return; > > - if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) > - return; > - > - nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > - hash = le32_to_cpu(nic_mdid->rss_hash); > - skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > + if (ice_copy_rx_hash_from_desc(rx_desc, &hash)) likely()? I wouldn't care about zero-hashed frames, their perf is not critical anyway. > + skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > } > > /** > @@ -186,7 +202,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring, > union ice_32b_rx_flex_desc *rx_desc, > struct sk_buff *skb, u16 ptype) > { > - ice_rx_hash(rx_ring, rx_desc, skb, ptype); > + ice_rx_hash_to_skb(rx_ring, rx_desc, skb, ptype); > > /* modifies the skb - consumes the enet header */ > skb->protocol = eth_type_trans(skb, rx_ring->netdev); Thanks, Olek
On Fri, May 19, 2023 at 06:46:31PM +0200, Alexander Lobakin wrote: > From: Larysa Zaremba <larysa.zaremba@intel.com> > Date: Fri, 12 May 2023 17:25:53 +0200 > > > Previously, we only needed RX hash 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. > > > > Separate generic process of reading RX hash from a descriptor > > into a separate function. > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 38 +++++++++++++------ > > 1 file changed, 27 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > index c8322fb6f2b3..fc67bbf600af 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > @@ -63,28 +63,44 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) > > } > > > > /** > > - * ice_rx_hash - set the hash value in the skb > > + * ice_copy_rx_hash_from_desc - copy hash value from descriptor to address > > + * @rx_desc: specific descriptor > > + * @dst: address to copy hash value to > > + * > > + * Returns true, if valid hash has been copied into the destination address. > > + */ > > +static bool > > +ice_copy_rx_hash_from_desc(union ice_32b_rx_flex_desc *rx_desc, u32 *dst) > > @rx_desc can be const. Yes > > I'm also unsure about the naming. Why not name this one ice_rx_hash() > and the one which sets it in skb ice_rx_hash_skb()? I just think that ice_copy_rx_hash_from_desc(desc, &hash, ...); ice_copy_rx_hash_from_desc(desc, hash_ptr, ...); communicates the intention (for a person that does not see a prototype) much better than ice_rx_hash(desc, &hash, ...); ice_rx_hash(desc, hash_ptr, ...); But now when I think about that, 'from_desc' part can probably be dropped without little to no impact, if we also replace 'copy' with sth more descriptive, like: ice_read_rx_hash(desc, &hash, ...); ice_read_rx_hash(desc, hash_ptr, ...); Same for timestamp functions. Probably, the main reason I started naming functions this way was ice_get_vlan_tag_from_rx_desc(). '_from_rx_desc' part is pretty redundant there too. I won't change '_to_skb' part though, I think function should show the direction of change it applies. > > > +{ > > + struct ice_32b_rx_flex_desc_nic *nic_mdid; > > Also const. I thought you'll pick most of my optimizations from the > related commit :D Well, at some point I kinda forgot about the patch, because it wasn't very usefult at the start of development, to be honest. Should have looked at it the the later stages though >_< Will make nic_mdid const. > > > + > > + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) > > + return false; > > + > > + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > > + *dst = le32_to_cpu(nic_mdid->rss_hash); > > + return true; > > You can just return the hash. `hash == 0` means there's no hash, so it > basically means `false`, while non-zero is `true`. Agree about both hash and timestamp. Taking this comment and the earlier on into account, I'll name functions like that: ice_get_rx_hash() ice_get_vlan_tag() ice_ptp_get_rx_hwts_ns() > > > +} > > + > > +/** > > + * ice_rx_hash_to_skb - set the hash value in the skb > > * @rx_ring: descriptor ring > > * @rx_desc: specific descriptor > > * @skb: pointer to current skb > > * @rx_ptype: the ptype value from the descriptor > > */ > > static void > > -ice_rx_hash(struct ice_rx_ring *rx_ring, union ice_32b_rx_flex_desc *rx_desc, > > - struct sk_buff *skb, u16 rx_ptype) > > +ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring, > > + union ice_32b_rx_flex_desc *rx_desc, > > + struct sk_buff *skb, u16 rx_ptype) > > { > > - struct ice_32b_rx_flex_desc_nic *nic_mdid; > > u32 hash; > > > > if (!(rx_ring->netdev->features & NETIF_F_RXHASH)) > > return; > > > > - if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) > > - return; > > - > > - nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > > - hash = le32_to_cpu(nic_mdid->rss_hash); > > - skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > + if (ice_copy_rx_hash_from_desc(rx_desc, &hash)) > > likely()? I wouldn't care about zero-hashed frames, their perf is not > critical anyway. Sure. > > > + skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > } > > > > /** > > @@ -186,7 +202,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring, > > union ice_32b_rx_flex_desc *rx_desc, > > struct sk_buff *skb, u16 ptype) > > { > > - ice_rx_hash(rx_ring, rx_desc, skb, ptype); > > + ice_rx_hash_to_skb(rx_ring, rx_desc, skb, ptype); > > > > /* modifies the skb - consumes the enet header */ > > skb->protocol = eth_type_trans(skb, rx_ring->netdev); > > Thanks, > Olek
From: Larysa Zaremba <larysa.zaremba@intel.com> Date: Mon, 22 May 2023 17:03:54 +0200 > On Fri, May 19, 2023 at 06:46:31PM +0200, Alexander Lobakin wrote: >> From: Larysa Zaremba <larysa.zaremba@intel.com> >> Date: Fri, 12 May 2023 17:25:53 +0200 [...] >>> + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; >>> + *dst = le32_to_cpu(nic_mdid->rss_hash); >>> + return true; >> >> You can just return the hash. `hash == 0` means there's no hash, so it >> basically means `false`, while non-zero is `true`. > > Agree about both hash and timestamp. > > Taking this comment and the earlier on into account, I'll name functions like > that: > > ice_get_rx_hash() > ice_get_vlan_tag() > ice_ptp_get_rx_hwts_ns() Sounds good to me! [...] 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 c8322fb6f2b3..fc67bbf600af 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c @@ -63,28 +63,44 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) } /** - * ice_rx_hash - set the hash value in the skb + * ice_copy_rx_hash_from_desc - copy hash value from descriptor to address + * @rx_desc: specific descriptor + * @dst: address to copy hash value to + * + * Returns true, if valid hash has been copied into the destination address. + */ +static bool +ice_copy_rx_hash_from_desc(union ice_32b_rx_flex_desc *rx_desc, u32 *dst) +{ + struct ice_32b_rx_flex_desc_nic *nic_mdid; + + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) + return false; + + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; + *dst = le32_to_cpu(nic_mdid->rss_hash); + return true; +} + +/** + * ice_rx_hash_to_skb - set the hash value in the skb * @rx_ring: descriptor ring * @rx_desc: specific descriptor * @skb: pointer to current skb * @rx_ptype: the ptype value from the descriptor */ static void -ice_rx_hash(struct ice_rx_ring *rx_ring, union ice_32b_rx_flex_desc *rx_desc, - struct sk_buff *skb, u16 rx_ptype) +ice_rx_hash_to_skb(struct ice_rx_ring *rx_ring, + union ice_32b_rx_flex_desc *rx_desc, + struct sk_buff *skb, u16 rx_ptype) { - struct ice_32b_rx_flex_desc_nic *nic_mdid; u32 hash; if (!(rx_ring->netdev->features & NETIF_F_RXHASH)) return; - if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) - return; - - nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; - hash = le32_to_cpu(nic_mdid->rss_hash); - skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); + if (ice_copy_rx_hash_from_desc(rx_desc, &hash)) + skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); } /** @@ -186,7 +202,7 @@ ice_process_skb_fields(struct ice_rx_ring *rx_ring, union ice_32b_rx_flex_desc *rx_desc, struct sk_buff *skb, u16 ptype) { - ice_rx_hash(rx_ring, rx_desc, skb, ptype); + ice_rx_hash_to_skb(rx_ring, rx_desc, skb, ptype); /* modifies the skb - consumes the enet header */ skb->protocol = eth_type_trans(skb, rx_ring->netdev);
Previously, we only needed RX hash 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. Separate generic process of reading RX hash from a descriptor into a separate function. Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 38 +++++++++++++------ 1 file changed, 27 insertions(+), 11 deletions(-)