> 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?
If not I can merge.
BR
Ian
>
> > Thanks
> > Ian
> >>
> >> Signed-off-by: Kevin Traynor <ktray...@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
> >> d...@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
