Re: [PATCH RESEND bpf-next 01/15] ice: make RX hash reading code more reusable

[Larysa Zaremba]

On Fri, May 19, 2023 at 06:46:31PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> 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@xxxxxxxxx>
> > ---
> >  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