> On 15/02/2021 13:06, Ilya Maximets wrote: > > On 2/13/21 9:33 AM, Stokes, Ian wrote: > >>> On 12/02/2021 17:42, Stokes, Ian wrote: > >>>>> In order for auto load balance to be enabled, there are > >>>>> minimum requirements of more than one PMD and more than > >>>>> one Rxq on at least one PMD. > >>>>> > >>>>> If these conditions are not met a rebalance would be pointless, > >>>>> so auto load balance is not enabled. > >>>>> > >>>>> Currently the state is logged but in the case where the criteria > >>>>> for enabling is not met, there is no reason given. > >>>>> > >>>>> It would be useful for the user to see the reason, so they > >>>>> can understand why auto load balance has not been enabled > >>>>> when they have requested it. > >>>>> > >>>>> For example, if a user has one PMD and sets pmd-auto-lb=true, > >>>>> previously: > >>>>> |INFO|PMD auto load balance is disabled > >>>>> > >>>>> With patch: > >>>>> |INFO|PMD auto load balance not enough PMDs or Rx Queues to enable > >>>>> |INFO|PMD auto load balance is disabled > >>>> > >>>> Thanks for the patch Kevin. > >>>> > >>>> In testing this worked as expected. > >>>> > >>>> One query I had was did you give thought towards more detailed in the > log? > >>>> > >>> > >>> I hadn't thought about splitting them. They are both connected as > >>> increasing the PMDs, will reduce the number of RxQ's per PMD etc. so > >>> it's hard to be prescriptive in splitting them to tell the user exactly > >>> what they need to change. > >>> e.g. > >>> 2 rxq on 1 pmd "not enough PMDs" > >>> user increases pmds to 2 > >>> 1 rxq on 2 pmds "not enough rxqs" > >>> > >>> They'd get there in the end, but I wonder if it would be more annoying > >>> to get a new message after fixing the only one that was reported first > >>> time around :-) > >> > >> I hear ya, not nit picking. Pure spit balling on my side 😃. > >> > >>> > >>>> i.e. if its 1 PMD should we flag that PMD <=1 is an issue. > >>>> > >>>> Similar with the number of RXQs. > >>>> > >>>> Maybe that's overkill as you could argue the minimum requirements could > >>> change over time but if its something that could be flagged easily would > >>> it > be > >>> worth it? > >>>> > >>> > >>> At the moment it's easily added, with the caveat as per above that just > >>> saying pmds or rxqs alone may not give the full picture. You are right > >>> that the minimum requirements could change a bit in time, so we could > >>> end up with further interconnected min reqs which might make it more > >>> difficult to split. > >>> > >> > >> At the end of the day I suppose we are waiting on users to complain, to > >> date I > haven't those complaints it myself so I assume users are consulting the > documentation and are a ware of the minimum requirements WRT PMD and > RXQs. > >> > >>> If you think it's useful, I'm fine to add now, or else we can go with a > >>> single line now and review again later when there is more development on > >>> it and docs are improved etc. > >> > >> I think its better to go with this approach first. We can modify in the > >> future if > required. > >> > >> I'd like to see this and patch 1 of the series in OVS 2.15 if there are no > objections. > >> > >> @Ilya Maximets any objections to this? > > > > I'm OK with the patch #1, but this one makes me uncomfortable. > > > > The main thing that I don't really understand is why we're flipping > > on/off this feature based on a current rxq distribution at all? > > At some point, we need to know whether we want to rebalance things. So > we can check it once on enable/config change (current), or check it > every x mins, or ignore it and do a dry run anyway that will always > report not to do a rebalance. > > > The code here wasn't very clear from the start and gets more messy. > > I'd say that we should, probably, just remove this part and keep > > load balancing enabled or disabled just based on the user config. > > There is an argument to change the user interface for this. > > If a user enables when there is no point to do a rebalance (say 1 pmd), > the choice would be: > 1- log it is disabled and why it can't be enabled (current+patches) > 2- log it is enabled but that it won't "run" > 3- log it is enabled (Silently don't run, or let it run and report there > is nothing to do every x mins) > > 2 seems worst, as it exposes a third pseudo state. > > 3 would seem clearer on the surface as user/ovs state would always > match, but would waste some processing running when we already know > there is no point to run, or if we didn't run after saying enabled, it > might be confusing why it was missing from debug logs. > > > Otherwise, we will end up in some overly complicated logic here > > to print any meaningful log messages. > > > > ok, we can leave this patch for now, while we discuss further on the > options above. > > thanks, > Kevin.
OK, so it looks like I can apply patch 1 of the series to master/2.15 and leave this patch for further discussion? Regards Ian > > >> > >> If not I can merge. > >> > >> BR > >> Ian > >>> > >>>> Thanks > >>>> Ian > >>>>> > >>>>> Signed-off-by: Kevin Traynor <[email protected]> > >>>>> --- > >>>>> lib/dpif-netdev.c | 19 +++++++++++++++---- > >>>>> 1 file changed, 15 insertions(+), 4 deletions(-) > >>>>> > >>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > >>>>> index 4381c618f..833f45616 100644 > >>>>> --- a/lib/dpif-netdev.c > >>>>> +++ b/lib/dpif-netdev.c > >>>>> @@ -4213,4 +4213,5 @@ set_pmd_auto_lb(struct dp_netdev *dp, bool > >>>>> always_log) > >>>>> bool enable_alb = false; > >>>>> bool multi_rxq = false; > >>>>> + bool minreq = false; > >>>>> bool pmd_rxq_assign_cyc = dp->pmd_rxq_assign_cyc; > >>>>> > >>>>> @@ -4226,6 +4227,6 @@ set_pmd_auto_lb(struct dp_netdev *dp, bool > >>>>> always_log) > >>>>> } > >>>>> if (cnt && multi_rxq) { > >>>>> - enable_alb = true; > >>>>> - break; > >>>>> + minreq = true; > >>>>> + break; > >>>>> } > >>>>> cnt++; > >>>>> @@ -4233,6 +4234,5 @@ set_pmd_auto_lb(struct dp_netdev *dp, bool > >>>>> always_log) > >>>>> > >>>>> /* Enable auto LB if it is requested and cycle based assignment is > >>>>> true. > */ > >>>>> - enable_alb = enable_alb && pmd_rxq_assign_cyc && > >>>>> - pmd_alb->auto_lb_requested; > >>>>> + enable_alb = minreq && pmd_rxq_assign_cyc && pmd_alb- > >>>>>> auto_lb_requested; > >>>>> > >>>>> if (pmd_alb->is_enabled != enable_alb || always_log) { > >>>>> @@ -4251,4 +4251,15 @@ set_pmd_auto_lb(struct dp_netdev *dp, bool > >>>>> always_log) > >>>>> } else { > >>>>> pmd_alb->rebalance_poll_timer = 0; > >>>>> + if (pmd_alb->auto_lb_requested) { > >>>>> + if (!minreq) { > >>>>> + VLOG_INFO("PMD auto load balance not enough " > >>>>> + "PMDs or Rx Queues to enable"); > >>>>> + } > >>>>> + if (!pmd_rxq_assign_cyc) { > >>>>> + VLOG_INFO("PMD auto load balance needs " > >>>>> + "'other_config:pmd-rxq-assign=cycles' " > >>>>> + "to enable"); > >>>>> + } > >>>>> + } > >>>>> VLOG_INFO("PMD auto load balance is disabled"); > >>>>> } > >>>>> -- > >>>>> 2.26.2 > >>>>> > >>>>> _______________________________________________ > >>>>> dev mailing list > >>>>> [email protected] > >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>>> > >> > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
