> On 5 Oct 2023, at 21:50, Vitaliy Makkoveev <m...@openbsd.org> wrote:
>
> Otherwise `if_snd' could still contain packets. If any of them has
> associated data with bumped reference counter, the corresponding
> refcnt_finalize() will sleep forever. I don't think we need to
> transmit these packets when interface became running again.
there were a bunch of paths where the stack would implicitly take an interface
down and up again around the actual change it was trying to make. from memory
these kinds of things are disruptive anyway, so losing a few more packets might
not be a problem. we do ride this out at the moment though.
>
> I don't like this unlocked if_flags check we have in ifq_start_task().
> Guess READ_ONCE() is much better to load value.
there are no inconsistent intermediate values you can read from if_flags. are
you worried the compiler will load the value again and maybe we'll get a
different state the second time?
> The problem reported in the "wg destroy hangs" [1] thread is the kind of
> described above. Also, current ifq_start_task() implementation denies to
> introduce reference counters for wg_peer and remove this
> wg_peer_destroy() hack:
>
> NET_LOCK();
> while (!ifq_empty(&sc->sc_if.if_snd)) {
> NET_UNLOCK();
> tsleep_nsec(sc, PWAIT, "wg_ifq", 1000);
> NET_LOCK();
> }
> NET_UNLOCK();
>
> 1. https://marc.info/?t=169642396300001&r=1&w=2
>
> ok?
im kind of surprised that when dealing with a problem that only seem to appear
in wireguard that you lean toward ifqs needing a fix. especially surprising
when the wg code in question is a tsleep_nsec(1000) in a loop.
i would argue that wg go down, wait for the stack to stop using it, and then do
it's own purge.
>
> Index: sys/net/ifq.c
> ===================================================================
> RCS file: /cvs/src/sys/net/ifq.c,v
> retrieving revision 1.51
> diff -u -p -r1.51 ifq.c
> --- sys/net/ifq.c 5 Oct 2023 11:08:56 -0000 1.51
> +++ sys/net/ifq.c 5 Oct 2023 11:49:40 -0000
> @@ -140,8 +140,11 @@ ifq_start_task(void *p)
> struct ifqueue *ifq = p;
> struct ifnet *ifp = ifq->ifq_if;
>
> - if (!ISSET(ifp->if_flags, IFF_RUNNING) ||
> - ifq_empty(ifq) || ifq_is_oactive(ifq))
> + if (!ISSET(READ_ONCE(ifp->if_flags), IFF_RUNNING)){
> + ifq_purge(ifq);
> + return;
> + }
> + if (ifq_empty(ifq) || ifq_is_oactive(ifq))
> return;
>
> ifp->if_qstart(ifq);
>
