> On 6 Jul 2020, at 17:36, Martin Pieuchot <[email protected]> wrote:
>
> On 06/07/20(Mon) 16:42, Vitaliy Makkoveev wrote:
>> [...]
>> pipex(4) is simultaneously locked by NET_LOCK() and KERNEL_LOCK() but
>> with two exceptions:
>>
>> 1. As you pointed pipex_pppoe_input() called without KERNEL_LOCK() held.
>> 2. pppac_start() called without NET_LOCK() held. Or with NET_LOCK()
>> held. It depends on `if_snd' usage.
>>
>> Diff below enforces pppac_start() to be called with NET_LOCK() held.
>> Also all externally called pipex(4) input and output routines have
>> NET_ASSERT_LOCKED() assertion.
>>
>> Now pipex(4) is fully protected by NET_LOCK() so description of struct
>> members chenget too.
>>
>> Index: sys/net/if_pppx.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/if_pppx.c,v
>> retrieving revision 1.90
>> diff -u -p -r1.90 if_pppx.c
>> --- sys/net/if_pppx.c 24 Jun 2020 08:52:53 -0000 1.90
>> +++ sys/net/if_pppx.c 6 Jul 2020 11:10:17 -0000
>> @@ -1117,6 +1117,8 @@ pppacopen(dev_t dev, int flags, int mode
>> ifp->if_output = pppac_output;
>> ifp->if_start = pppac_start;
>> ifp->if_ioctl = pppac_ioctl;
>> + /* XXXSMP: be sure pppac_start() called under NET_LOCK() */
>> + IFQ_SET_MAXLEN(&ifp->if_snd, 1);
>
> Is it possible to grab the NET_LOCK() inside pppac_start() instead of
> grabbing it outside? This should allow *start() routine to be called
> from any context.
Unfortunately you can’t be sure about NET_LOCK() status while you are
in pppac_start(). It was described at this thread [1].
We have two cases:
1. pppac_start() called from pppac_output(). NET_LOCK() was inherited.
2. pppac_start() called from `systq’. There is no NET_LOCK() held.
NET_LOCK() is not required for pipex_{,ppp}_output() because underlay
routines were switched to ip{,6}_send().
I intentionally want to made all pipex(4) locked by one lock and be
sure it’s locking is consistent. I hope to start implementing fine
graining locks just after pipex(4) be switched to if_get(9).
>
> It might be interesting to see that as a difference between the NET_LOCK()
> used to protect the network stack internals and the NET_LOCK() used to
> protect pipex(4) internals. Such distinction might help to convert the
> latter into a different lock or primitive.
>
>> Index: sys/net/pipex.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/pipex.c,v
>> retrieving revision 1.117
>> diff -u -p -r1.117 pipex.c
>> --- sys/net/pipex.c 30 Jun 2020 14:05:13 -0000 1.117
>> +++ sys/net/pipex.c 6 Jul 2020 11:10:17 -0000
>> @@ -869,6 +869,7 @@ pipex_output(struct mbuf *m0, int af, in
>> struct ip ip;
>> struct mbuf *mret;
>>
>> + NET_ASSERT_LOCKED();
>
> This function doesn't touch any shared data structure, we'd better move
> the NET_ASSERT_LOCKED() above rn_lookuo() in pipex_lookup_by_ip_address().
>
> Note that `pipex_rd_head4' and `pipex_rd_head6' are, with this diff,
> also protected by the NET_LOCK() and should be annotated as such.
>
>> session = NULL;
>> mret = NULL;
>> switch (af) {
>> @@ -962,6 +963,8 @@ pipex_ppp_output(struct mbuf *m0, struct
>> {
>> u_char *cp, hdr[16];
>>
>> + NET_ASSERT_LOCKED();
>
> Same here, it seems that the only reason the NET_LOCK() is necessary in
> the output path is to prevent corruption of the `session' descriptor being
> used. So we'd rather put the assertion above the LIST_FOREACH().
They touch `session->stat’ :) We can switch pipex(4) to percpu counters and
output will be lockless. But I guess this should be done later.
>
> Anyway all of those can be addressed later, your diff is ok mpi@
>
Thanks. I will commit it if no one has objections.
1. https://marc.info/?t=158998611500002&r=1&w=2
