> On 30 Dec 2020, at 01:18, Claudio Jeker <[email protected]> wrote:
>
> On Tue, Dec 29, 2020 at 08:48:28PM +0100, Klemens Nanni wrote:
>> Earlier this year `struct pppoe_softc' was annotated with lock comments
>> showing no member being protected by KERNEL_LOCK() alone.
>>
>> After further review of the code paths starting from pppoeintr() I also
>> could not find sleeping points, which must be avoided in the sofnet
>> thread. (As part of this, I specifically went through all possible
>> malloc(9) calls and verified that none of them use `M_WAITOK'.)
>>
>> The only thing I see neccessary now is wrapping mbuf queue access with
>> if_get(9) to prevent interfaces from disappearing while processing the
>> mbuf -- currently the big lock protects against that.
>>
>> I've been running with this diff for over four months on an octeon
>> edgerouter 4 which updates to snapshots modulo the custom kernel roughly
>> once a week.
>>
>> Others also successfully tested this on octeon and amd64 based setups.
>>
>> Did I miss anything?
>> Feedback? Objections? OK?
>
> Generally I would prefer to go for direct dispatch and not use netisr.
> This removes a queue and a scheduling point and should help reduce the
> latency in processing pppoe packages.
This makes sense.
>
> This does not mean that I'm against this change. I just think it may be
> benefitial to move one step further.
>
>> Index: if.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/if.c,v
>> retrieving revision 1.621
>> diff -u -p -r1.621 if.c
>> --- if.c 15 Dec 2020 03:43:34 -0000 1.621
>> +++ if.c 28 Dec 2020 18:13:02 -0000
>> @@ -907,9 +907,7 @@ if_netisr(void *unused)
>> #endif
>> #if NPPPOE > 0
>> if (n & (1 << NETISR_PPPOE)) {
>> - KERNEL_LOCK();
>> pppoeintr();
>> - KERNEL_UNLOCK();
>> }
>> #endif
>> t |= n;
>> Index: if_pppoe.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/if_pppoe.c,v
>> retrieving revision 1.73
>> diff -u -p -r1.73 if_pppoe.c
>> --- if_pppoe.c 13 Sep 2020 11:00:40 -0000 1.73
>> +++ if_pppoe.c 28 Dec 2020 18:13:02 -0000
>> @@ -346,14 +346,29 @@ void
>> pppoeintr(void)
>> {
>> struct mbuf *m;
>> + struct ifnet *ifp;
>>
>> NET_ASSERT_LOCKED();
>>
>> - while ((m = niq_dequeue(&pppoediscinq)) != NULL)
>> + while ((m = niq_dequeue(&pppoediscinq)) != NULL) {
>> + ifp = if_get(m->m_pkthdr.ph_ifidx);
>> + if (ifp == NULL) {
>> + m_freem(m);
>> + continue;
>> + }
>> pppoe_disc_input(m);
>> + if_put(ifp);
>> + }
>>
>> - while ((m = niq_dequeue(&pppoeinq)) != NULL)
>> + while ((m = niq_dequeue(&pppoeinq)) != NULL) {
>> + ifp = if_get(m->m_pkthdr.ph_ifidx);
>> + if (ifp == NULL) {
>> + m_freem(m);
>> + continue;
>> + }
>> pppoe_data_input(m);
>> + if_put(ifp);
>> + }
>> }
>>
>> /* Analyze and handle a single received packet while not in session state. */
>>
>
> --
> :wq Claudio
