[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:02:11 UTC 2021


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