On Thu, Mar 25, 2021 at 7:10 PM John Fastabend [off-list ref] wrote:

Hi Cong,

I'm trying to understand if the workqueue logic will somehow prevent the
following,

  CPU0                         CPU1

 work dequeue
 sk_psock_backlog()
    ... do backlog
    ... also maybe sleep

                               schedule_work()
                               work_dequeue
                               sk_psock_backlog()

          <----- multiple runners -------->

 work_complete

It seems we could get multiple instances of sk_psock_backlog(), unless
the max_active is set to 1 in __queue_work() which would push us through
the WORK_STRUCT_DELAYED state. At least thats my initial read. Before
it didn't matter because we had the sock_lock to ensure we have only a
single runner here.

I need to study the workqueue code here to be sure, but I'm thinking
this might a problem unless we set up the workqueue correctly.

Do you have any extra details on why above can't happen thanks.

Very good question!

I thought a same work callback is never executed concurrently, but
after reading the workqueue code, actually I agree with you on this, that
is, a same work callback can be executed concurrently on different CPU's.

Limiting max_active to 1 is not a solution here, as we still want to keep
different items running concurrently. Therefore, we still need a mutex here,
just to protect this scenario. I will add a psock->work_mutex inside
sk_psock_backlog().

Thanks!