| Message ID | 20230602141307.661043-3-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Improve linux QoS for exotic and fast links | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2 Jun 2023, at 16:13, Adrian Moreno wrote: > Instead of relying on feature bits, use the speed value directly as > maximum rate for htb and hfsc classes. > > There is still a limitation with the maximum rate that we can express > with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed > instead of the feature bits, we can at least use an accurate maximum for > some link speeds (such as 25Gbps) which are not supported by netdev's feature > bits. Hi Adrian, See some comments below. //Eelco > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > lib/netdev-linux.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 3055f88d2..56b487eea 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_, > > hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; > if (!hc->max_rate) { > - enum netdev_features current; > - > netdev_linux_read_features(netdev); > - current = !netdev->get_features_error ? netdev->current : 0; > - hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; Re-reading my previous comments on fallback for to netdev_get_features() I guess the new API should replace this one. So to make a statement I would say try to remove the netdev_features_to_bps() API, or move it to netdev_bsd_features_to_bps() for netdev_bsd_get_speed only (with a comment) so it’s clear that people should NOT use this API from now on. > + hc->max_rate = !netdev->get_features_error > + ? netdev->current_speed / 8 * 1000000ULL > + : NETDEV_DEFAULT_BPS / 8; What if for some reason netdev->current_speed equals zero, it no longer reports NETDEV_DEFAULT_BPS / 8. > } > hc->min_rate = hc->max_rate; > hc->burst = 0; > @@ -5218,11 +5217,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap *details, > > uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; > if (!max_rate) { > - enum netdev_features current; > - > netdev_linux_read_features(netdev); > - current = !netdev->get_features_error ? netdev->current : 0; > - max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; > + max_rate = !netdev->get_features_error > + ? netdev->current_speed / 8 * 1000000ULL > + : NETDEV_DEFAULT_BPS / 8; Same as above when netdev->current_speed == 0. > } > > class->min_rate = max_rate; > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 7/6/23 14:50, Eelco Chaudron wrote: > > > On 2 Jun 2023, at 16:13, Adrian Moreno wrote: > >> Instead of relying on feature bits, use the speed value directly as >> maximum rate for htb and hfsc classes. >> >> There is still a limitation with the maximum rate that we can express >> with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed >> instead of the feature bits, we can at least use an accurate maximum for >> some link speeds (such as 25Gbps) which are not supported by netdev's feature >> bits. > > Hi Adrian, > > See some comments below. > > //Eelco > > >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> lib/netdev-linux.c | 14 ++++++-------- >> 1 file changed, 6 insertions(+), 8 deletions(-) >> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >> index 3055f88d2..56b487eea 100644 >> --- a/lib/netdev-linux.c >> +++ b/lib/netdev-linux.c >> @@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_, >> >> hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >> if (!hc->max_rate) { >> - enum netdev_features current; >> - >> netdev_linux_read_features(netdev); >> - current = !netdev->get_features_error ? netdev->current : 0; >> - hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; > > Re-reading my previous comments on fallback for to netdev_get_features() I guess the new API should replace this one. > So to make a statement I would say try to remove the netdev_features_to_bps() API, or move it to netdev_bsd_features_to_bps() for netdev_bsd_get_speed only (with a comment) so it’s clear that people should NOT use this API from now on. > We are still not deprecating the old API (netdev_get_features() and "enum netdev_features") since it is in line with OFP1.0 that does not have numeric speeds. e.g: lib/ofp-port uses netdev_get_features() to convert the maximum speed to kbps when decoding OFP1.0 port descriptions. What I do think is a good idea is to add a comment pointing new users to the new API if what they are looking for is a more accurate speed. WDYT? >> + hc->max_rate = !netdev->get_features_error >> + ? netdev->current_speed / 8 * 1000000ULL >> + : NETDEV_DEFAULT_BPS / 8; > > What if for some reason netdev->current_speed equals zero, it no longer reports NETDEV_DEFAULT_BPS / 8. > Although it's likely this never happens with the current implementation, this could change and your proposal is more robust. I'll update it. >> } >> hc->min_rate = hc->max_rate; >> hc->burst = 0; >> @@ -5218,11 +5217,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap *details, >> >> uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >> if (!max_rate) { >> - enum netdev_features current; >> - >> netdev_linux_read_features(netdev); >> - current = !netdev->get_features_error ? netdev->current : 0; >> - max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; >> + max_rate = !netdev->get_features_error >> + ? netdev->current_speed / 8 * 1000000ULL >> + : NETDEV_DEFAULT_BPS / 8; > > Same as above when netdev->current_speed == 0. > >> } >> >> class->min_rate = max_rate; >> -- >> 2.40.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 7/10/23 16:19, Adrian Moreno wrote: > > > On 7/6/23 14:50, Eelco Chaudron wrote: >> >> >> On 2 Jun 2023, at 16:13, Adrian Moreno wrote: >> >>> Instead of relying on feature bits, use the speed value directly as >>> maximum rate for htb and hfsc classes. >>> >>> There is still a limitation with the maximum rate that we can express >>> with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed >>> instead of the feature bits, we can at least use an accurate maximum for >>> some link speeds (such as 25Gbps) which are not supported by netdev's feature >>> bits. >> >> Hi Adrian, >> >> See some comments below. >> >> //Eelco >> >> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>> --- >>> lib/netdev-linux.c | 14 ++++++-------- >>> 1 file changed, 6 insertions(+), 8 deletions(-) >>> >>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >>> index 3055f88d2..56b487eea 100644 >>> --- a/lib/netdev-linux.c >>> +++ b/lib/netdev-linux.c >>> @@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_, >>> >>> hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >>> if (!hc->max_rate) { >>> - enum netdev_features current; >>> - >>> netdev_linux_read_features(netdev); >>> - current = !netdev->get_features_error ? netdev->current : 0; >>> - hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; >> >> Re-reading my previous comments on fallback for to netdev_get_features() I >> guess the new API should replace this one. >> So to make a statement I would say try to remove the netdev_features_to_bps() >> API, or move it to netdev_bsd_features_to_bps() for netdev_bsd_get_speed only >> (with a comment) so it’s clear that people should NOT use this API from now on. >> > > We are still not deprecating the old API (netdev_get_features() and "enum > netdev_features") since it is in line with OFP1.0 that does not have numeric > speeds. e.g: lib/ofp-port uses netdev_get_features() to convert the maximum > speed to kbps when decoding OFP1.0 port descriptions. > > What I do think is a good idea is to add a comment pointing new users to the new > API if what they are looking for is a more accurate speed. WDYT? > > Or maybe we could even rename it and add "legacy" in the name so that it's more clear that it should not be used (for those who don't read the comments :-)).
On 10 Jul 2023, at 16:26, Adrian Moreno wrote: > On 7/10/23 16:19, Adrian Moreno wrote: >> >> >> On 7/6/23 14:50, Eelco Chaudron wrote: >>> >>> >>> On 2 Jun 2023, at 16:13, Adrian Moreno wrote: >>> >>>> Instead of relying on feature bits, use the speed value directly as >>>> maximum rate for htb and hfsc classes. >>>> >>>> There is still a limitation with the maximum rate that we can express >>>> with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed >>>> instead of the feature bits, we can at least use an accurate maximum for >>>> some link speeds (such as 25Gbps) which are not supported by netdev's feature >>>> bits. >>> >>> Hi Adrian, >>> >>> See some comments below. >>> >>> //Eelco >>> >>> >>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>> --- >>>> lib/netdev-linux.c | 14 ++++++-------- >>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >>>> index 3055f88d2..56b487eea 100644 >>>> --- a/lib/netdev-linux.c >>>> +++ b/lib/netdev-linux.c >>>> @@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_, >>>> >>>> hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >>>> if (!hc->max_rate) { >>>> - enum netdev_features current; >>>> - >>>> netdev_linux_read_features(netdev); >>>> - current = !netdev->get_features_error ? netdev->current : 0; >>>> - hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; >>> >>> Re-reading my previous comments on fallback for to netdev_get_features() I guess the new API should replace this one. >>> So to make a statement I would say try to remove the netdev_features_to_bps() API, or move it to netdev_bsd_features_to_bps() for netdev_bsd_get_speed only (with a comment) so it’s clear that people should NOT use this API from now on. >>> >> >> We are still not deprecating the old API (netdev_get_features() and "enum netdev_features") since it is in line with OFP1.0 that does not have numeric speeds. e.g: lib/ofp-port uses netdev_get_features() to convert the maximum speed to kbps when decoding OFP1.0 port descriptions. >> >> What I do think is a good idea is to add a comment pointing new users to the new API if what they are looking for is a more accurate speed. WDYT? >> >> > > Or maybe we could even rename it and add "legacy" in the name so that it's more clear that it should not be used (for those who don't read the comments :-)). What about a check patch warning when the API is used? > -- > Adrián Moreno
On 7/10/23 16:33, Eelco Chaudron wrote: > > > On 10 Jul 2023, at 16:26, Adrian Moreno wrote: > >> On 7/10/23 16:19, Adrian Moreno wrote: >>> >>> >>> On 7/6/23 14:50, Eelco Chaudron wrote: >>>> >>>> >>>> On 2 Jun 2023, at 16:13, Adrian Moreno wrote: >>>> >>>>> Instead of relying on feature bits, use the speed value directly as >>>>> maximum rate for htb and hfsc classes. >>>>> >>>>> There is still a limitation with the maximum rate that we can express >>>>> with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed >>>>> instead of the feature bits, we can at least use an accurate maximum for >>>>> some link speeds (such as 25Gbps) which are not supported by netdev's feature >>>>> bits. >>>> >>>> Hi Adrian, >>>> >>>> See some comments below. >>>> >>>> //Eelco >>>> >>>> >>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >>>>> --- >>>>> lib/netdev-linux.c | 14 ++++++-------- >>>>> 1 file changed, 6 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c >>>>> index 3055f88d2..56b487eea 100644 >>>>> --- a/lib/netdev-linux.c >>>>> +++ b/lib/netdev-linux.c >>>>> @@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_, >>>>> >>>>> hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; >>>>> if (!hc->max_rate) { >>>>> - enum netdev_features current; >>>>> - >>>>> netdev_linux_read_features(netdev); >>>>> - current = !netdev->get_features_error ? netdev->current : 0; >>>>> - hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; >>>> >>>> Re-reading my previous comments on fallback for to netdev_get_features() I guess the new API should replace this one. >>>> So to make a statement I would say try to remove the netdev_features_to_bps() API, or move it to netdev_bsd_features_to_bps() for netdev_bsd_get_speed only (with a comment) so it’s clear that people should NOT use this API from now on. >>>> >>> >>> We are still not deprecating the old API (netdev_get_features() and "enum netdev_features") since it is in line with OFP1.0 that does not have numeric speeds. e.g: lib/ofp-port uses netdev_get_features() to convert the maximum speed to kbps when decoding OFP1.0 port descriptions. >>> >>> What I do think is a good idea is to add a comment pointing new users to the new API if what they are looking for is a more accurate speed. WDYT? >>> >>> >> >> Or maybe we could even rename it and add "legacy" in the name so that it's more clear that it should not be used (for those who don't read the comments :-)). > > What about a check patch warning when the API is used? > OK. I'll look into checkpatch.py. Thanks. >> -- >> Adrián Moreno >
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 3055f88d2..56b487eea 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -4746,11 +4746,10 @@ htb_parse_qdisc_details__(struct netdev *netdev_, hc->max_rate = smap_get_ullong(details, "max-rate", 0) / 8; if (!hc->max_rate) { - enum netdev_features current; - netdev_linux_read_features(netdev); - current = !netdev->get_features_error ? netdev->current : 0; - hc->max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; + hc->max_rate = !netdev->get_features_error + ? netdev->current_speed / 8 * 1000000ULL + : NETDEV_DEFAULT_BPS / 8; } hc->min_rate = hc->max_rate; hc->burst = 0; @@ -5218,11 +5217,10 @@ hfsc_parse_qdisc_details__(struct netdev *netdev_, const struct smap *details, uint32_t max_rate = smap_get_ullong(details, "max-rate", 0) / 8; if (!max_rate) { - enum netdev_features current; - netdev_linux_read_features(netdev); - current = !netdev->get_features_error ? netdev->current : 0; - max_rate = netdev_features_to_bps(current, NETDEV_DEFAULT_BPS) / 8; + max_rate = !netdev->get_features_error + ? netdev->current_speed / 8 * 1000000ULL + : NETDEV_DEFAULT_BPS / 8; } class->min_rate = max_rate;
Instead of relying on feature bits, use the speed value directly as maximum rate for htb and hfsc classes. There is still a limitation with the maximum rate that we can express with a 32-bit number in bytes/s (~ 34.3Gbps), but using the actual link speed instead of the feature bits, we can at least use an accurate maximum for some link speeds (such as 25Gbps) which are not supported by netdev's feature bits. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- lib/netdev-linux.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)