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. > > 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
