Ilya Maximets <[email protected]> writes: > If someone creates a lot of meters (>1017) in the kernel datapath > and then re-starts ovs-vswitchd, OVS fails to probe meter support > and refuses to install any meters afterwards: > > dpif_netlink|INFO|The kernel module has a broken meter implementation. > > The reason is that probing for broken meters relies on creating > two meters with high meter IDs. Inside the kernel, however, the > meter table is not a real hash table since v5.10 and commit: > > c7c4c44c9a95 ("net: openvswitch: expand the meters supported number") > > Instead, it's just an array with meter IDs mapped to indexes with a > simple modulo operation. This array can expand, but only when it is > full. There is no handling of collisions, so if the meter at > ID % size is not the right one, then the lookup just fails. This is > fine as long as userspace creates meters with densely packed IDs, > which is the case for ovs-vswitchd... except for probing. > > While probing, we attempt to create meters 54545401 and 54545402. > Without expanding the table size, these map onto 1017 and 1018. > So, creation of these meters fails if there are already meters with > IDs 1017 or 1018. At this point OVS declares meter implementation > broken.
Just a comment that it shouldn't actually matter what the meter IDs are, right? For example, you change from the very high values, but it actually doesn't affect whether the meter ID check is working. The big issue is that we're not really allowing for the case that a meter already exists. > Ideally, we should make the "hash" table in the kernel handle > collisions and otherwise be a more or less proper hash table. But > we can also improve probing in userspace and avoid this issue by > choosing lower numbered meter IDs and trying to get them from the > kernel first before trying to create. Choosing high values at the > top of the 0-1023 range, so they are guaranteed to fit into the > minimal size table in the kernel (1024). If one of these already > exists and has a proper ID, then meters are likely working fine > and we don't need to install new ones for probing. If these meters > are not in the kernel, then we can try to create and check. > > This logic should work fine for older or future kernels with a > proper hash table as well. > > There is no Fixes tag here as the check was correct at the moment > it was introduced. It's the kernel change that broke it. > > Reported-at: https://github.com/openvswitch/ovs-issues/issues/337 > Signed-off-by: Ilya Maximets <[email protected]> > --- Acked-by: Aaron Conole <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
