On Sat, Dec 04, 2021 at 08:40:42PM +0100, Stefan Sperling wrote:
> On Sat, Dec 04, 2021 at 09:32:40PM +0300, Vitaliy Makkoveev wrote:
> > I think rtm_80211info() could follow the if_link_state_change()
> > way and use task for that.
>
> Indeed. I did not realize that if_link_state_change() schedules a task.
>
> This means ieee80211_set_link_state() is already deferring some of its
> work to a task.
Probably should have mentioned this in my original mail.
> The patch below defers sending the 80211info message to a task as well.
>
> I am keeping things simple for now and use systq (kernel-locked) instead
> of copying the argument-passing approach used by if_link_state_change().
All timeouts run under the kernel lock, so this is appropriate.
> Tested on iwm(4) 8265 with 'route monitor'.
>
> ok?
I like the direction. I have a few concerns noted below.
> diff 0b61c8235787960f0010ef627ea5b2c6309a81f0
> de98c050ea709bdb8e26be40ab0cc82ef9afed80
> blob - 7bb68194dd78417b06c59f81d1ebbff4165203d8
> blob + 5b9a969258074fde29e21a33ac035cf170ec3b03
> --- sys/net80211/ieee80211.c
> +++ sys/net80211/ieee80211.c
> @@ -193,6 +193,7 @@ ieee80211_ifattach(struct ifnet *ifp)
> if_addgroup(ifp, "wlan");
> ifp->if_priority = IF_WIRELESS_DEFAULT_PRIORITY;
>
> + task_set(&ic->ic_rtm_80211info_task, ieee80211_rtm_80211info_task, ic);
> ieee80211_set_link_state(ic, LINK_STATE_DOWN);
>
> timeout_set(&ic->ic_bgscan_timeout, ieee80211_bgscan_timeout, ifp);
> @@ -203,6 +204,7 @@ ieee80211_ifdetach(struct ifnet *ifp)
> {
> struct ieee80211com *ic = (void *)ifp;
>
> + task_del(systq, &ic->ic_rtm_80211info_task);
> timeout_del(&ic->ic_bgscan_timeout);
Suppose the ic_bgscan_timeout timeout is running at the moment we're
running ieee80211_ifdetach(). Ignore the kernel lock for the moment,
think about the future.
If we delete the task before we delete the timeout and the timeout
then adds the task back onto the task queue, what happens?
My guess is you need to ensure the timeout is no longer running
*before* you delete the task. Can you do timeout_del_barrier()
here? See the attached patch.
>
> /*
> blob - 447a2676bfb250b7f917206549679d6ae68de1f6
> blob + 7e10fc1336067542c13d5607602e658ce2b3926b
> --- sys/net80211/ieee80211_proto.c
> +++ sys/net80211/ieee80211_proto.c
> @@ -1288,6 +1288,31 @@ justcleanup:
> }
>
> void
> +ieee80211_rtm_80211info_task(void *arg)
> +{
> + struct ieee80211com *ic = arg;
> + struct ifnet *ifp = &ic->ic_if;
> + struct if_ieee80211_data ifie;
> + int s = splnet();
> +
> + if (LINK_STATE_IS_UP(ifp->if_link_state)) {
Does this mean userspace can "miss" state transitions if the task runs
and the state has already changed back to not-up?
I don't know whether this would matter in practice, but it would be a
behavior change.
--
The rest of the patch looks good, running with it now on this:
$ dmesg | grep iwm0 | tail -n2
iwm0 at pci1 dev 0 function 0 "Intel Dual Band Wireless-AC 8265" rev 0x78, msi
iwm0: hw rev 0x230, fw ver 36.ca7b901d.0, address 98:3b:8f:ef:6b:ef
Unsure how `route monitor` exercises this path, but I've left it
running, too.
Index: ieee80211.c
===================================================================
RCS file: /cvs/src/sys/net80211/ieee80211.c,v
retrieving revision 1.85
diff -u -p -r1.85 ieee80211.c
--- ieee80211.c 11 Oct 2021 09:01:06 -0000 1.85
+++ ieee80211.c 5 Dec 2021 17:01:51 -0000
@@ -193,6 +193,7 @@ ieee80211_ifattach(struct ifnet *ifp)
if_addgroup(ifp, "wlan");
ifp->if_priority = IF_WIRELESS_DEFAULT_PRIORITY;
+ task_set(&ic->ic_rtm_80211info_task, ieee80211_rtm_80211info_task, ic);
ieee80211_set_link_state(ic, LINK_STATE_DOWN);
timeout_set(&ic->ic_bgscan_timeout, ieee80211_bgscan_timeout, ifp);
@@ -203,7 +204,8 @@ ieee80211_ifdetach(struct ifnet *ifp)
{
struct ieee80211com *ic = (void *)ifp;
- timeout_del(&ic->ic_bgscan_timeout);
+ timeout_del_barrier(&ic->ic_bgscan_timeout);
+ task_del(systq, &ic->ic_rtm_80211info_task);
/*
* Undo pseudo-driver changes. Pseudo-driver detach hooks could
