[ovs-dev] [PATCH 2/2] dpif-netdev: auto load balance improve enable/disable logs.
Stokes, Ian
ian.stokes at intel.com
Mon Feb 15 16:20:32 UTC 2021
> 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 <ktraynor at redhat.com>
> >>>>>>> ---
> >>>>>>> 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
> >>>>>>> dev at openvswitch.org
> >>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>>>>>
> >>>>
> >>>
> >
More information about the dev
mailing list