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@xxxxxxxxx>
> > ---
>
> (...)
>
> > +
> > +/**
> > + * 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
> >
