| Message ID | 20210205165659.1405585-2-ktraynor@redhat.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | [ovs-dev,1/2] dpif-netdev: auto load balance log state on user request. | expand |
> 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.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? Thanks Ian > > Signed-off-by: Kevin Traynor <ktraynor@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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.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. 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. > Thanks > Ian >> >> Signed-off-by: Kevin Traynor <ktraynor@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@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> 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 <ktraynor@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@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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? 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. Otherwise, we will end up in some overly complicated logic here to print any meaningful log messages. > > If not I can merge. > > BR > Ian >> >>> Thanks >>> Ian >>>> >>>> Signed-off-by: Kevin Traynor <ktraynor@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@openvswitch.org >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >
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. >> >> If not I can merge. >> >> BR >> Ian >>> >>>> Thanks >>>> Ian >>>>> >>>>> Signed-off-by: Kevin Traynor <ktraynor@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@openvswitch.org >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >> >
> 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@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@openvswitch.org > >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>>> > >> > >
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 <ktraynor@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@openvswitch.org >>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> >>>> >>> >
> 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@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@openvswitch.org > >>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>>>>> > >>>> > >>> > >
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.
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"); }
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 Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- lib/dpif-netdev.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)