[xdp-hints] Re: [PATCH bpf-next v6 06/18] ice: Support RX hash XDP hint

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Fri, Oct 20, 2023 at 05:27:34PM +0200, Maciej Fijalkowski wrote:
> On Thu, Oct 12, 2023 at 07:05:12PM +0200, Larysa Zaremba wrote:
> > RX hash XDP hint requests both hash value and type.
> > Type is XDP-specific, so we need a separate way to map
> > these values to the hardware ptypes, so create a lookup table.
> > 
> > Instead of creating a new long list, reuse contents
> > of ice_decode_rx_desc_ptype[] through preprocessor.
> > 
> > Current hash type enum does not contain ICMP packet type,
> > but ice devices support it, so also add a new type into core code.
> > 
> > Then use previously refactored code and create a function
> > that allows XDP code to read RX hash.
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> 
> (...)
> 
> > +
> > +/**
> > + * ice_xdp_rx_hash_type - Get XDP-specific hash type from the RX descriptor
> > + * @eop_desc: End of Packet descriptor
> > + */
> > +static enum xdp_rss_hash_type
> > +ice_xdp_rx_hash_type(const union ice_32b_rx_flex_desc *eop_desc)
> > +{
> > +	u16 ptype = ice_get_ptype(eop_desc);
> > +
> > +	if (unlikely(ptype >= ICE_NUM_DEFINED_PTYPES))
> > +		return 0;
> > +
> > +	return ice_ptype_to_xdp_hash[ptype];
> > +}
> > +
> > +/**
> > + * ice_xdp_rx_hash - RX hash XDP hint handler
> > + * @ctx: XDP buff pointer
> > + * @hash: hash destination address
> > + * @rss_type: XDP hash type destination address
> > + *
> > + * Copy RX hash (if available) and its type to the destination address.
> > + */
> > +static int ice_xdp_rx_hash(const struct xdp_md *ctx, u32 *hash,
> > +			   enum xdp_rss_hash_type *rss_type)
> > +{
> > +	const struct ice_xdp_buff *xdp_ext = (void *)ctx;
> > +
> > +	*hash = ice_get_rx_hash(xdp_ext->pkt_ctx.eop_desc);
> > +	*rss_type = ice_xdp_rx_hash_type(xdp_ext->pkt_ctx.eop_desc);
> > +	if (!likely(*hash))
> > +		return -ENODATA;
> 
> maybe i have missed previous discussions, but why hash/rss_type are copied
> regardless of them being available? if i am missing something can you
> elaborate on that?
> 
> also, !likely() construct looks tricky to me, I am not sure what was the
> intent behind it. other callbacks return -ENODATA in case NETIF_F_RXHASH
> is missing from dev->features.
>

Well, we get RX hash in the descriptor regardless of whether NETIF_F_RXHASH is 
enabled (I have tested this), so no point in checking this in the hints 
functions.

Regarding `!likely(*hash)`: we have already discussed that valid `hash == 0` is 
very improbable, so there is no harm in treating it as a failure for the sake of 
consistency with other hints functions. Basically I treat `hash == 0` here as 
"no hash in the descriptor".

But there is an error in this code snippet that I see now: there also must be a 
check that `rss_type != 0`, otherwise packet has an unhashable type, which must 
result in `-ENODATA`.
 
> > +
> > +	return 0;
> > +}
> > +
> >  const struct xdp_metadata_ops ice_xdp_md_ops = {
> >  	.xmo_rx_timestamp		= ice_xdp_rx_hw_ts,
> > +	.xmo_rx_hash			= ice_xdp_rx_hash,
> >  };
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 349c36fb5fd8..eb77040b4825 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -427,6 +427,7 @@ enum xdp_rss_hash_type {
> >  	XDP_RSS_L4_UDP		= BIT(5),
> >  	XDP_RSS_L4_SCTP		= BIT(6),
> >  	XDP_RSS_L4_IPSEC	= BIT(7), /* L4 based hash include IPSEC SPI */
> > +	XDP_RSS_L4_ICMP		= BIT(8),
> >  
> >  	/* Second part: RSS hash type combinations used for driver HW mapping */
> >  	XDP_RSS_TYPE_NONE            = 0,
> > @@ -442,11 +443,13 @@ enum xdp_rss_hash_type {
> >  	XDP_RSS_TYPE_L4_IPV4_UDP     = XDP_RSS_L3_IPV4 | XDP_RSS_L4 | XDP_RSS_L4_UDP,
> >  	XDP_RSS_TYPE_L4_IPV4_SCTP    = XDP_RSS_L3_IPV4 | XDP_RSS_L4 | XDP_RSS_L4_SCTP,
> >  	XDP_RSS_TYPE_L4_IPV4_IPSEC   = XDP_RSS_L3_IPV4 | XDP_RSS_L4 | XDP_RSS_L4_IPSEC,
> > +	XDP_RSS_TYPE_L4_IPV4_ICMP    = XDP_RSS_L3_IPV4 | XDP_RSS_L4 | XDP_RSS_L4_ICMP,
> >  
> >  	XDP_RSS_TYPE_L4_IPV6_TCP     = XDP_RSS_L3_IPV6 | XDP_RSS_L4 | XDP_RSS_L4_TCP,
> >  	XDP_RSS_TYPE_L4_IPV6_UDP     = XDP_RSS_L3_IPV6 | XDP_RSS_L4 | XDP_RSS_L4_UDP,
> >  	XDP_RSS_TYPE_L4_IPV6_SCTP    = XDP_RSS_L3_IPV6 | XDP_RSS_L4 | XDP_RSS_L4_SCTP,
> >  	XDP_RSS_TYPE_L4_IPV6_IPSEC   = XDP_RSS_L3_IPV6 | XDP_RSS_L4 | XDP_RSS_L4_IPSEC,
> > +	XDP_RSS_TYPE_L4_IPV6_ICMP    = XDP_RSS_L3_IPV6 | XDP_RSS_L4 | XDP_RSS_L4_ICMP,
> >  
> >  	XDP_RSS_TYPE_L4_IPV6_TCP_EX  = XDP_RSS_TYPE_L4_IPV6_TCP  | XDP_RSS_L3_DYNHDR,
> >  	XDP_RSS_TYPE_L4_IPV6_UDP_EX  = XDP_RSS_TYPE_L4_IPV6_UDP  | XDP_RSS_L3_DYNHDR,
> > -- 
> > 2.41.0
> >