On Mon, May 21, 2018 at 10:59:36AM +0000, Shahaf Shuler wrote:
> Hi Adrien,
>
> Please help me walkthrough this logic. Something doesn't end up correctly.
>
> Friday, May 18, 2018 12:49 PM, Adrien Mazarguil:
> > Cc: Shahaf Shuler <shah...@mellanox.com>; dev@dpdk.org
> > Subject: Re: [PATCH 2/2] net/mlx4: refactor RSS conversion functions
> > > > Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> > > > Cc: Ophir Munk <ophi...@mellanox.com>
> > > > *
> > > > * @return
> > > > - * A valid Verbs RSS hash fields mask for mlx4 on success,
> > > > (uint64_t)-1
> > > > - * otherwise and rte_errno is set.
> > > > + * Converted RSS hash fields on success, (uint64_t)-1 otherwise and
> > > > + * rte_errno is set.
> > > > */
> > > > uint64_t
> > > > -mlx4_conv_rss_types(struct priv *priv, uint64_t types)
> > > > +mlx4_conv_rss_types(struct priv *priv, uint64_t types, int
> > > > +verbs_to_dpdk)
> > > > {
>
>
> Let's assume:
> Types = IBV_RX_HASH_SRC_IPV4 | IBV_RX_HASH_DST_IPV4
> And verbs_to_dpdk=1
>
> > > > - enum { IPV4, IPV6, TCP, UDP, };
> > > > - static const uint64_t in[] = {
> > > > + enum {
> > > > + INNER, IPV4, IPV6, TCP, UDP,
> > > > + IPV4_TCP, IPV4_UDP, IPV6_TCP, IPV6_UDP,
> > > > + };
> > > > + static const uint64_t dpdk[] = {
> > > > + [INNER] = 0,
> > > > [IPV4] = (ETH_RSS_IPV4 |
> > > > ETH_RSS_FRAG_IPV4 |
> > > > - ETH_RSS_NONFRAG_IPV4_TCP |
> > > > - ETH_RSS_NONFRAG_IPV4_UDP |
> > > > ETH_RSS_NONFRAG_IPV4_OTHER),
> > > > [IPV6] = (ETH_RSS_IPV6 |
> > > > ETH_RSS_FRAG_IPV6 |
> > > > - ETH_RSS_NONFRAG_IPV6_TCP |
> > > > - ETH_RSS_NONFRAG_IPV6_UDP |
> > > > ETH_RSS_NONFRAG_IPV6_OTHER |
> > > > - ETH_RSS_IPV6_EX |
> > > > - ETH_RSS_IPV6_TCP_EX |
> > > > - ETH_RSS_IPV6_UDP_EX),
> > > > - [TCP] = (ETH_RSS_NONFRAG_IPV4_TCP |
> > > > - ETH_RSS_NONFRAG_IPV6_TCP |
> > > > - ETH_RSS_IPV6_TCP_EX),
> > > > - [UDP] = (ETH_RSS_NONFRAG_IPV4_UDP |
> > > > - ETH_RSS_NONFRAG_IPV6_UDP |
> > > > - ETH_RSS_IPV6_UDP_EX),
> > > > + ETH_RSS_IPV6_EX),
> > > > + [TCP] = 0,
> > > > + [UDP] = 0,
> > > > + [IPV4_TCP] = ETH_RSS_NONFRAG_IPV4_TCP,
> > > > + [IPV4_UDP] = ETH_RSS_NONFRAG_IPV4_UDP,
> > > > + [IPV6_TCP] = (ETH_RSS_NONFRAG_IPV6_TCP |
> > > > + ETH_RSS_IPV6_TCP_EX),
> > > > + [IPV6_UDP] = (ETH_RSS_NONFRAG_IPV6_UDP |
> > > > + ETH_RSS_IPV6_UDP_EX),
> > > > };
> > > > - static const uint64_t out[RTE_DIM(in)] = {
> > > > + const uint64_t verbs[RTE_DIM(dpdk)] = {
> > > > + [INNER] = IBV_RX_HASH_INNER,
> > > > [IPV4] = IBV_RX_HASH_SRC_IPV4 |
> > IBV_RX_HASH_DST_IPV4,
> > > > [IPV6] = IBV_RX_HASH_SRC_IPV6 |
> > IBV_RX_HASH_DST_IPV6,
> > > > [TCP] = IBV_RX_HASH_SRC_PORT_TCP |
> > IBV_RX_HASH_DST_PORT_TCP,
> > > > [UDP] = IBV_RX_HASH_SRC_PORT_UDP |
> > IBV_RX_HASH_DST_PORT_UDP,
> > > > + [IPV4_TCP] = verbs[IPV4] | verbs[TCP],
> > > > + [IPV4_UDP] = verbs[IPV4] | verbs[UDP],
> > > > + [IPV6_TCP] = verbs[IPV6] | verbs[TCP],
> > > > + [IPV6_UDP] = verbs[IPV6] | verbs[UDP],
> > > > };
> > > > + const uint64_t *in = verbs_to_dpdk ? verbs : dpdk;
> > > > + const uint64_t *out = verbs_to_dpdk ? dpdk : verbs;
> > > > uint64_t seen = 0;
> > > > uint64_t conv = 0;
> > > > unsigned int i;
> > > >
> > > > - if (!types)
>
> Types != 0 , we skip.
>
> > > > - return priv->hw_rss_sup;
> > > > - for (i = 0; i != RTE_DIM(in); ++i)
> > > > + if (!types) {
> > > > + if (!verbs_to_dpdk)
> > > > + return priv->hw_rss_sup;
> > > > + types = priv->hw_rss_sup;
> > > > + }
> > > > + for (i = 0; i != RTE_DIM(dpdk); ++i)
> > > > if (types & in[i]) {
> > > > seen |= types & in[i];
> > > > conv |= out[i];
> > > > }
>
>
> After this loop:
> seen = (IBV_RX_HASH_SRC_PORT_IPV4 | IBV_RX_HASH_DST_IPV4)
> conv = dpdk[IPV4] | dpdk[IPV4_TCP] | dpdk[IPV4_UDP]
Now I understand the mistake.
> > > > - if ((conv & priv->hw_rss_sup) == conv && !(types & ~seen))
> > > > + if ((verbs_to_dpdk || (conv & priv->hw_rss_sup) == conv) &&
> > > > + !(types & ~seen))
>
> All types were seen && verbs_to_dpdk hence returning that PMD support RSS
> based on IPV4 | TCPV4 | UDPV4
> While according to the reported capabilities form kernel only IPV4 is
> supported.
Indeed, although this combination never occurs in practice, that's why I
didn't see any issue at first. I'll submit a fixed version. Thanks.
> > > > return conv;
> > > > rte_errno = ENOTSUP;
> > > > return (uint64_t)-1;
> > > > }
> > > >
> > >
--
Adrien Mazarguil
6WIND
