On 22/05/20(Fri) 13:25, Vitaliy Makkoveev wrote:
> On Fri, May 22, 2020 at 07:57:13AM +1000, David Gwynne wrote:
> > [...]
> > can you try the following diff?
> >
>
> I tested this diff and it works for me. But the problem I pointed is
> about pipex(4) locking.
>
> pipex(4) requires NET_LOCK() be grabbed not only for underlaying
> ip{,6}_output() but for itself too. But since pppac_start() has
> unpredictable behavior I suggested to make it predictable [1].
What needs the NET_LOCK() in their? We're talking about
pipex_ppp_output(), right? Does it really need the NET_LOCK() or
the KERNEL_LOCK() is what protects those data structures?
> With net/ifq.c rev 1.37 and rev 1.39 pppx_if_start() routine will be
> allways called under NET_LOCK(), but pppac_start() will not. It depends
> on packet count stored in `if_snd' (look at net/ifq.c:125).
Ideally the *_start() routine should not require the NET_LOCK().
Ideally the pseudo-drivers should not require the NET_LOCK(). That's
what we should aim for.
In case of pipex(4) is isn't clear that the NET_LOCK() is necessary.
> With net/ifq.c rev 1.38 pppx_if_start() and pppac_start() have fully
> identical behavior and we can simply add NET_LOCK() [2] required by
> pipex(4).
>
> Also I can write my own handler for `ifp->if_enqueue' to be sure about
> `ifp->if_start' locking state, but I still have misunderstanding why
> pppx(4) does "IFQ_SET_MAXLEN(&ifp->if_snd, 1)". Just to make locking
> state of pppx_if_start() predictable or for avoid hidden bug triggering?
>
> Also this time I'am not shure about KERNEL_LOCK() locking while pipex(4)
> is accessed from the stack so I can't say is NET_LOCK() really required
> for the whole pipex(4).
>
> 1. https://marc.info/?l=openbsd-tech&m=159006455720473&w=2
> 2. https://marc.info/?l=openbsd-tech&m=158998597126002&w=2
>
