On 5/5/23 12:39, Adrian Moreno wrote: > > > On 5/5/23 11:56, Ilya Maximets wrote: >> On 5/5/23 10:04, Adrian Moreno wrote: >>> >>> >>> On 5/4/23 17:52, Ilya Maximets wrote: >>>> On 4/21/23 17:16, Adrian Moreno wrote: >>>>> Currently, the netdev's speed is being calculated by taking the link's >>>>> feature bits (using netdev_get_features()) and transforming them into >>>>> bps. >>>>> >>>>> This mechanism can be both inaccurate and difficult to maintain, mainly >>>>> because we currently use the feature bits supported by OpenFlow which >>>>> would have to be extended to support all new feature bits of all netdev >>>>> implementations while keeping the OpenFlow API intact. >>>>> >>>>> In order to expose the link speed accurately for all current and future >>>>> hardware, add a new netdev API call that allows the implementations to >>>>> provide the current and maximum link speeds in Mbps. >>>>> >>>>> Internally, the logic to get the maximum supported speed still relies on >>>>> feature bits so it might still get out of sync in the future. However, >>>>> the maximum configurable speed is not used as much as the current speed >>>>> and these feature bits are not exposed through the netdev interface so >>>>> it should be easier to add more. >>>>> >>>>> Use this new function instead of netdev_get_features() where the link >>>>> speed is needed. >>>>> >>>>> As a consecuence of this patch, link speeds of cards is properly >>>>> reported (internally in OVSDB) even if not supported by OpenFlow. >>>> >>>> Hi, Adrian. Thanks for the set! I didn't test it, but I have a few >>>> comments inline. >>>> >>>> Best regards, Ilya Maximets. >>>> >>>>> >>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137567 >>>> >>>> I'm not sure about this link. The BZ talks about switching to >>>> ETHTOOL_GLINKSETTINGS interface to get speeds that do not have >>>> predefined values. Current patch is a step forward to be able >>>> to support other link speeds, but IIUC, we will still not able >>>> to detect 25 and 50 Gbps links in netdev-linux. >>> >>> It's true that we will not be able to report a bit in the feature bitmask >>> that indicates 25 or 50 Gbps. I think this is not a huge deal because we >>> don't have those flags in the ofproto layer anyway. >>> >>> However, with this patch netdev-linux should be able to read the speed >>> directly from ethtool_cmd_speed() which uses a 32-bit field in Mbps. Using >>> this speed directly (and not the bitmask) to calculate QoS maximum rates >>> and even to report the speed in ofproto port information (OFP >= 1.1) >>> should, IIUC, address the limitations reported in the BZ. >> >> Oh, I see. I missed the fact that ETHTOOL_GSET reports a numerical >> value of the speed. >> >> Is it possible to add a system test for this functionality? e.g. can >> we change the speed of a tap or veth interface with ethtool to some >> currently unsupported value (50G) ? >> >> OTOH, ETHTOOL_GSET is still a deprecated interface, so we'll need to >> eventually move to ETHTOOL_GLINKSETTINGS. But that can be a separate >> change. >> > > That's true. However, there is a compatibility layer so even if new drivers > don't implement the old driver API, it'll still be usable from uapi. What the > new API definitely improves the amount (and extensibility) of feature bits. > > But as long as the compatibility layer properly reports the numeric speed, we > will be accurate in reporting the link speed (as well as calculating QoS max > rates, etc). > > Now, if we evaluate the benefit OVS will take from the new API, I think it > won't be very big since OFP layer will not support the new feature bits. The > only real benefit would be increased accuracy in the numeric maximum speed > reported since the only way is to calculate this based on the feature bits. > My feeling is that it's not that big of a benefit compared with the > complexity of having to maintain both APIs, what do you think?
I agree that there is no much benefit today. And I would not expect the interface to actually be removed any time soon, even though it's clearly marked as deprecated. We can revisit this later, if needed. BTW, do we need a get_speed() implementation for netdev-bsd ? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
