Hi Johannes,
On 8/30/19 5:14 AM, Johannes Berg wrote:
On Fri, 2019-08-16 at 14:27 -0500, Denis Kenzior wrote:
Send large NEW_WIPHY events on a new multicast group so that clients
that can accept larger messages do not need to round-trip to the kernel
and perform extra filtered wiphy dumps.
A new multicast group is introduced and the large message is sent before
the legacy message. This way clients that listen on both multicast
groups can ignore duplicate legacy messages if needed.
Since I just did the digging, it seems that this would affect (old)
applications with libnl up to 3.2.22, unless they changed the default
recvmsg() buffer size.
Sorry, I'm not sure I understand. Are you saying new clients would try
to use old libnl and subscribe to this new multicast group for large
messages? Legacy clients shouldn't even see messages on this multicast
group since they would never subscribe to it.
I think this is a pretty decent approach, but I'm slightly worried about
hitting the new limits (16k) eventually. It seems far off now, but who
knows what kind of data we'll add. HE is (and likely will be) adding
quite a bit since it has everything for each interface type - something
drivers have for the most part not implemented yet. That trend will only
continue, as complexity in the spec doesn't seem to be going down.
Right, but the kernel will go up to 32k buffers if userspace read buffer
is that large. So I think we have quite some room to grow. On the
other hand, we probably should be vigilant that any new stuff added
tries to minimize message sizes whenever possible.
And I don't really want to see "config3" a couple of years down the
road...
Agreed.
So can we at least mandate (document) that "config2" basically has no
message limit, and you will use MSG_PEEK/handle MSG_TRUNC with it?
Yes, I will take care of that in v3.
That way, we can later bump the 8192 even beyond 16k if needed, and not
run into problems.
+ if (cmd == NL80211_CMD_NEW_WIPHY) {
+ state.large_message = true;
+ alloc_size = 8192UL;
+ } else
+ alloc_size = NLMSG_DEFAULT_SIZE;
+
nit: there should be braces on both branches
will fix
+ if (nl80211_send_wiphy(rdev, cmd, msg, 0, 0, 0, &state) < 0) {
+ nlmsg_free(msg);
+ goto legacy;
+ }
I think that'd be worth a WARN_ON(), it should never happen that you
actually run out of space, it means that the above wasn't big enough.
Yep
Now, on the previous patches I actually thought that you could set
"state->split" (and you should) and not need "state->large_message" in
order to indicate that the sub-functions are allowed to create larger
data - just keep filling the SKBs as much as possible for the dump.
Here, it seems like we do need it. It might be possible to get away
without it (by setting split here, and then having some special code to
handle the case of it not getting to the end), but that doesn't seem
worth it.
@@ -14763,6 +14787,8 @@ void nl80211_notify_iface(struct cfg80211_registered_device *rdev,
return;
}
+ genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
+ NL80211_MCGRP_CONFIG2, GFP_KERNEL);
Hmm. That seems only needed if you don't want to listen on "config" at
all, but in the patch description you explicitly said that you send it
on "config2" *before* "config" for compatibility reasons (which makes
sense) - so what is it?
Well it can be both, depending on whether large messages can fail or
not. So one use case might be that a client detects whether the config2
multicast group exists. If so, then it only subscribes to it and that's it.
Another use case might be (if userspace is worried about losing large
messages) to subscribe to both groups. If it receives the large
message, it can ignore the one that comes on the legacy multicast group.
I'm having a hard time seeing anyone get away with only listening on
config2 since that'd basically require very recent (as of now future)
kernel. Are you planning this for a world where you can ditch support
for kernel<5.4 (or so)?
No, but there's nothing stopping the client in making the choice at
runtime depending on the genl family info it gets. E.g. by peeking into
CTRL_ATTR_MCAST_GROUPS.
Regards,
-Denis
