Hey Kevin,
Kevin Traynor, May 24, 2023 at 16:13:
> Hi Robin,
>
> I tested combinations of enabling/disabling cp-proto and enabling
> hwol. It is working as expected and hwol feature always has a clear
> priority, regardless of the order they are enabled in.
>
> I didn't test lacp traffic, but I was able to see that the rxq was
> added/removed for it and that rss was working on the other rxqs.
>
> I also tested combinations of different numbers of rxqs,
> adding/deleting rxqs, enabling/disabling cp-proto etc and working
> fine. I also tested enabling cp-proto and changing rxqs in the same
> command, working fine.
Thanks for testing!
> > + /* User input for n_rxq + an optional control plane protection
> > queue
> > + * (see netdev_dpdk_reconfigure). This field is different from the
> > + * other requested_* fields as it may contain a different value
> > than
> > + * the user input. */
>
> ^ I don't think this comment is really needed. requested_rxq_size can
> also be adjusted and they don't always come from user input, sometimes
> just the OVS default.
...
> > + /* User input for n_rxq (see dpdk_set_rxq_config). */
> > + int user_n_rxq;
>
> I also put in a similar 3rd stage status in recent patches for
> rx/tx-descriptors but managed to get rid of it. You might be able to
> remove this and just check check cp-proto flags do a +1/-1 where
> needed?
I had already tried this and it was not possible. I will have another
fresh look. Maybe I was thinking about this the wrong way.
> > + /*
> > + * Some drivers set reta_size equal to the total number of rxqs
> > that
> > + * are configured when it is a power of two. Since we are actually
> > + * reconfiguring the redirection table to exclude the last rxq, we
> > may
> > + * end up with an imbalanced redirection table. For example, such
> > + * configuration:
> > + *
> > + * options:n_rxq=3 options:cp-protection=lacp
> > + *
> > + * Will actually configure 4 rxqs on the NIC, and the default reta
> > to:
> > + *
> > + * [0, 1, 2, 3]
> > + *
> > + * And dpdk_cp_prot_rss_configure() will reconfigure reta to:
> > + *
> > + * [0, 1, 2, 0]
> > + *
> > + * Causing queue 0 to receive twice as much traffic as queues 1
> > and 2.
> > + *
> > + * Work around that corner case by forcing a bigger redirection
> > table
> > + * size to 128 entries when reta_size is not a multiple of
> > rss_n_rxq
> > + * and when reta_size is less than 128. This value seems to be
> > + * supported by most of the drivers that also support rte flow.
> > + */
> > + info.reta_size = RTE_ETH_RSS_RETA_SIZE_128;
>
> Thanks for following up on previous comments and adding this
> workaround. I think you didn't get much response about it, so might be
> worth adding a DPDK bugzilla to formally report/track it.
I don't think it is a bug. If the default redirection table is not
modified, there will be proper balancing on all Rx queues. The issue we
have here is that we are reconfiguring the table and removing the last
queue from it which can lead to imbalances in some cases. Do you think
this is worth reporting anyway?
> > + try_cp_prot = dev->requested_cp_prot_flags != 0;
> > + dev->requested_n_rxq = dev->user_n_rxq;
> > + if (try_cp_prot) {
> > + dev->requested_n_rxq += 1;
>
> Minor, but adjusting the requested_n_rxq's seems more suited to
> dpdk_cp_prot_set_config() (and move it after dpdk_set_rxq_config() in
> netdev_dpdk_set_config()) as this is the function that applies the
> configuration. I admit, it's partly aesthetic, so that this function
> still just checks for changes that require a reconfig and returns if
> there are none :-)
This is related to the introduction of user_n_rxq. I had tried to make
it work and this was the only way. I'll have another fresh look.
Cheers,
Robin
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
