> On 2/15/21 5:02 PM, Stokes, Ian wrote: > >> 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? > > Looks like that, yes.
Great, thanks for all the input folks, patch 1 pushed to master and 2.15. BR Ian > > > > > 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
