| Message ID | 20230824192703.712881-2-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:40PM +0200, Larysa Zaremba wrote: > 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 | 37 +++++++++++++------ > 1 file changed, 26 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..8f7f6d78f7bf 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -63,28 +63,43 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) > } > > /** > - * ice_rx_hash - set the hash value in the skb > + * ice_get_rx_hash - get RX hash value from descriptor > + * @rx_desc: specific descriptor > + * > + * Returns hash, if present, 0 otherwise. > + */ > +static u32 > +ice_get_rx_hash(const union ice_32b_rx_flex_desc *rx_desc) > +{ > + const struct ice_32b_rx_flex_desc_nic *nic_mdid; > + > + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) > + return 0; > + > + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > + return le32_to_cpu(nic_mdid->rss_hash); > +} > + > +/** > + * 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(const struct ice_rx_ring *rx_ring, nit: maybe ice_rx_skb_hash, but i have not seen xdp side yet. other idea would be to turn ice_get_rx_hash to __ice_rx_hash and keep the ice_rx_hash name as-is. Usual way of naming internal funcs. Take it or leave it:) > + const 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)); > + hash = ice_get_rx_hash(rx_desc); > + if (likely(hash)) > + skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); Looks like a behavior change as you wouldn't be setting l4_hash and sw_hash from skb in case !hash ? When can we get hash == 0 ? > } > > /** > @@ -186,7 +201,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); > -- > 2.41.0 >
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Date: Mon, 4 Sep 2023 16:37:45 +0200 > On Thu, Aug 24, 2023 at 09:26:40PM +0200, Larysa Zaremba wrote: >> 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. [...] >> - 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)); >> + hash = ice_get_rx_hash(rx_desc); >> + if (likely(hash)) >> + skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); > > Looks like a behavior change as you wouldn't be setting l4_hash and > sw_hash from skb in case !hash ? When can we get hash == 0 ? I do the same in libie. hash == 0 makes no sense at all no matter if you set sw or l4, esp. for GRO and other stack pieces. BTW, sw_hash is never set by drivers, it's meant to be set only from the core networking hashing functions (when it's hashed by CPU with SIPhash with the help of Flow Dissector). So we only do care about l4_hash. Valid hash == 0 for valid L4 frame has 0.0(0)1% probability even for XOR, not speaking of Toeplitz / CRC (have you even seen MD5 == 0? :D). if the frame is not L4, the kernel doesn't treat your hash as something meaningful and falls back to SIP. But the prob of having hash == 0 for L3- is not higher :D > >> } >> >> /** >> @@ -186,7 +201,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); >> -- >> 2.41.0 >> Thanks, Olek
From: Larysa Zaremba <larysa.zaremba@intel.com> Date: Thu, 24 Aug 2023 21:26:40 +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> I like the patch, except three minors above, > --- > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 37 +++++++++++++------ > 1 file changed, 26 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..8f7f6d78f7bf 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > @@ -63,28 +63,43 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) > } > > /** > - * ice_rx_hash - set the hash value in the skb > + * ice_get_rx_hash - get RX hash value from descriptor > + * @rx_desc: specific descriptor > + * > + * Returns hash, if present, 0 otherwise. > + */ > +static u32 > +ice_get_rx_hash(const union ice_32b_rx_flex_desc *rx_desc) The whole declaration could easily fit into one line :> > +{ > + const struct ice_32b_rx_flex_desc_nic *nic_mdid; > + > + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) Not really related: have you tried to measure branch hit/miss here? Can't it be a candidate for unlikely()? > + return 0; > + > + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > + return le32_to_cpu(nic_mdid->rss_hash); I think the common convention in the kernel is to separate the last return from the main body with a newline. To not leave the cast above alone, you can embed it into the declaration. const struct ice_32b_rx_flex_desc_nic *mdid = (typeof(mdid))rx_desc; This is a compile-time cast w/o any maths anyway, so doing it before checking for the descriptor type doesn't hurt in any way. if (!= FLEX) return 0; return le32_ ... (or via a ternary) > +} [...] Thanks, Olek
On Thu, Sep 14, 2023 at 06:12:23PM +0200, Alexander Lobakin wrote: > From: Larysa Zaremba <larysa.zaremba@intel.com> > Date: Thu, 24 Aug 2023 21:26:40 +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> > > I like the patch, except three minors above, > > > --- > > drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 37 +++++++++++++------ > > 1 file changed, 26 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..8f7f6d78f7bf 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c > > @@ -63,28 +63,43 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) > > } > > > > /** > > - * ice_rx_hash - set the hash value in the skb > > + * ice_get_rx_hash - get RX hash value from descriptor > > + * @rx_desc: specific descriptor > > + * > > + * Returns hash, if present, 0 otherwise. > > + */ > > +static u32 > > +ice_get_rx_hash(const union ice_32b_rx_flex_desc *rx_desc) > > The whole declaration could easily fit into one line :> > I agree > > +{ > > + const struct ice_32b_rx_flex_desc_nic *nic_mdid; > > + > > + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) > > Not really related: have you tried to measure branch hit/miss here? > Can't it be a candidate for unlikely()? I have not measured this, but at least in my test setup, I have never seen any other rxdid, so unlikely() is a good idea. If it harms some particular applications, we can always remove this later on request :D > > + return 0; > > + > > + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; > > + return le32_to_cpu(nic_mdid->rss_hash); > > I think the common convention in the kernel is to separate the last > return from the main body with a newline. > To not leave the cast above alone, you can embed it into the declaration. > I am fine with leaving the cast alone. > const struct ice_32b_rx_flex_desc_nic *mdid = (typeof(mdid))rx_desc; > > This is a compile-time cast w/o any maths anyway, so doing it before > checking for the descriptor type doesn't hurt in any way. > > if (!= FLEX) > return 0; > > return le32_ ... > > (or via a ternary) > > > +} > > [...] > > 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..8f7f6d78f7bf 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c @@ -63,28 +63,43 @@ static enum pkt_hash_types ice_ptype_to_htype(u16 ptype) } /** - * ice_rx_hash - set the hash value in the skb + * ice_get_rx_hash - get RX hash value from descriptor + * @rx_desc: specific descriptor + * + * Returns hash, if present, 0 otherwise. + */ +static u32 +ice_get_rx_hash(const union ice_32b_rx_flex_desc *rx_desc) +{ + const struct ice_32b_rx_flex_desc_nic *nic_mdid; + + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC) + return 0; + + nic_mdid = (struct ice_32b_rx_flex_desc_nic *)rx_desc; + return le32_to_cpu(nic_mdid->rss_hash); +} + +/** + * 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(const struct ice_rx_ring *rx_ring, + const 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)); + hash = ice_get_rx_hash(rx_desc); + if (likely(hash)) + skb_set_hash(skb, hash, ice_ptype_to_htype(rx_ptype)); } /** @@ -186,7 +201,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 | 37 +++++++++++++------ 1 file changed, 26 insertions(+), 11 deletions(-)