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
