On 2/15/21 5:20 PM, Stokes, Ian wrote: >> 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.
Thanks! I've sent 2.15 release patches for review. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
