Thanks for explanation. I missed the last commit to
sys/kern/uipc_socket.c
> On 10 Jul 2021, at 22:56, Martin Pieuchot <[email protected]> wrote:
>
> On 10/07/21(Sat) 21:53, Vitaliy Makkoveev wrote:
>> Hi,
>>
>> In filt_solisten_common() you touches `so_qlen’ only. It’s not
>> related to buffer and not protected by introduced `sb_mtx’ so
>> the solock() replacement in filt_solisten*() is wrong.
>>
>> However, in filt_solisten_common() you only checks is
>> `so_qlen’ != 0 condition and such check could be performed lockless.
>> I propose you to commit this by separate diff.
>
> The dance is there because knote needs a lock which could be the
> mutex.
>
>>> @@ -208,8 +208,10 @@ uipc_usrreq(struct socket *so, int req, struct mbuf
>>> *m, struct mbuf *nam,
>>> * Adjust backpressure on sender
>>> * and wakeup any waiting to write.
>>> */
>>> + mtx_enter(&so2->so_snd.sb_mtx);
>>> so2->so_snd.sb_mbcnt = so->so_rcv.sb_mbcnt;
>>> so2->so_snd.sb_cc = so->so_rcv.sb_cc;
>>> + mtx_leave(&so2->so_snd.sb_mtx);
>>> sowwakeup(so2);
>>> break;
>>>
>>
>> This is 'PRU_RCVD’ case, so you hold solock() on `so’ and it’s receive
>> buffer is locked by sblock(). Is it assumed `sb_mbcnt’ and `sb_cc’
>> modification of `so_rcv’ protected by solock()? Should the both buffers
>> be locked here? I’m asking because you only remove solock() from kqueue(9)
>> path and the solock() still serialises the rest of sockets, but you are
>> going to reduce solock().
>>
>> The same question for 'PRU_SEND’ case.
>
> In this case the fields of "so2" are modified. Modifications are always
> serialized by the solock. The mutex is there to prevent another thread
> running the kqueue filters to read between the update of `sb_mbcnt' and
> `sb_cc'.
>
>>> @@ -284,8 +286,10 @@ uipc_usrreq(struct socket *so, int req, struct mbuf
>>> *m, struct mbuf *nam,
>>> sbappendrecord(so2, &so2->so_rcv, m);
>>> else
>>> sbappend(so2, &so2->so_rcv, m);
>>> + mtx_enter(&so2->so_snd.sb_mtx);
>>> so->so_snd.sb_mbcnt = so2->so_rcv.sb_mbcnt;
>>> so->so_snd.sb_cc = so2->so_rcv.sb_cc;
>>> + mtx_leave(&so2->so_snd.sb_mtx);
>>> if (so2->so_rcv.sb_cc > 0)
>>> sorwakeup(so2);
>>
>>
>> Since you touch 'so2->so_rcv’ content here, you want to lock it instead
>> of 'so2->so_snd’, right?
>
> Indeed, that's a mistake, thanks!
>
