Re: [PATCH net 5/5] net/smc: put sk reference if close work was canceled

Wenjia Zhang
2023-10-23T20:52:28Z

On 23.10.23 14:18, D. Wythe wrote:
>
>
> On 10/23/23 6:28 PM, Wenjia Zhang wrote:
>>
>>
>> On 23.10.23 10:52, D. Wythe wrote:
>>>
>>>
>>> On 10/23/23 4:19 PM, Wenjia Zhang wrote:
>>>>
>>>>
>>>> On 20.10.23 04:41, D. Wythe wrote:
>>>>>
>>>>>
>>>>> On 10/20/23 1:40 AM, Wenjia Zhang wrote:
>>>>>>
>>>>>>
>>>>>> On 19.10.23 09:33, D. Wythe wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 10/19/23 4:26 AM, Wenjia Zhang wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 17.10.23 04:06, D. Wythe wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 10/13/23 3:04 AM, Wenjia Zhang wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 11.10.23 09:33, D. Wythe wrote:
>>>>>>>>>>> From: "D. Wythe" <alibuda@linux.alibaba.com>
>>>>>>>>>>>
>>>>>>>>>>> Note that we always hold a reference to sock when attempting
>>>>>>>>>>> to submit close_work.
>>>>>>>>>> yes
>>>>>>>>>> Therefore, if we have successfully
>>>>>>>>>>> canceled close_work from pending, we MUST release that reference
>>>>>>>>>>> to avoid potential leaks.
>>>>>>>>>>>
>>>>>>>>>> Isn't the corresponding reference already released inside the
>>>>>>>>>> smc_close_passive_work()?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Wenjia,
>>>>>>>>>
>>>>>>>>> If we successfully cancel the close work from the pending state,
>>>>>>>>> it means that smc_close_passive_work() has never been executed.
>>>>>>>>>
>>>>>>>>> You can find more details here.
>>>>>>>>>
>>>>>>>>> /**
>>>>>>>>> * cancel_work_sync - cancel a work and wait for it to finish
>>>>>>>>> * @work:the work to cancel
>>>>>>>>> *
>>>>>>>>> * Cancel @work and wait for its execution to finish. This function
>>>>>>>>> * can be used even if the work re-queues itself or migrates to
>>>>>>>>> * another workqueue. On return from this function, @work is
>>>>>>>>> * guaranteed to be not pending or executing on any CPU.
>>>>>>>>> *
>>>>>>>>> * cancel_work_sync(&delayed_work->work) must not be used for
>>>>>>>>> * delayed_work's. Use cancel_delayed_work_sync() instead.
>>>>>>>>> *
>>>>>>>>> * The caller must ensure that the workqueue on which @work was
>>>>>>>>> last
>>>>>>>>> * queued can't be destroyed before this function returns.
>>>>>>>>> *
>>>>>>>>> * Return:
>>>>>>>>> * %true if @work was pending, %false otherwise.
>>>>>>>>> */
>>>>>>>>> boolcancel_work_sync(structwork_struct *work)
>>>>>>>>> {
>>>>>>>>> return__cancel_work_timer(work, false);
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Best wishes,
>>>>>>>>> D. Wythe
>>>>>>>> As I understand, queue_work() would wake up the work if the work
>>>>>>>> is not already on the queue. And the sock_hold() is just prio to
>>>>>>>> the queue_work(). That means, cancel_work_sync() would cancel
>>>>>>>> the work either before its execution or after. If your fix
>>>>>>>> refers to the former case, at this moment, I don't think the
>>>>>>>> reference can be hold, thus it is unnecessary to put it.
>>>>>>>>>
>>>>>>>
>>>>>>> I am quite confuse about why you think when we cancel the work
>>>>>>> before its execution,
>>>>>>> the reference can not be hold ?
>>>>>>>
>>>>>>>
>>>>>>> Perhaps the following diagram can describe the problem in better
>>>>>>> way :
>>>>>>>
>>>>>>> smc_close_cancel_work
>>>>>>> smc_cdc_msg_recv_action
>>>>>>>
>>>>>>>
>>>>>>> sock_hold
>>>>>>> queue_work
>>>>>>> if (cancel_work_sync())        // successfully cancel before
>>>>>>> execution
>>>>>>> sock_put()                        //  need to put it since we
>>>>>>> already hold a ref before   queue_work()
>>>>>>>
>>>>>>>
>>>>>> ha, I already thought you might ask such question:P
>>>>>>
>>>>>> I think here two Problems need to be clarified:
>>>>>>
>>>>>> 1) Do you think the bh_lock_sock/bh_unlock_sock in the
>>>>>> smc_cdc_msg_recv does not protect the smc_cdc_msg_recv_action()
>>>>>> from cancel_work_sync()?
>>>>>> Maybe that would go back to the discussion in the other patch on
>>>>>> the behaviors of the locks.
>>>>>>
>>>>>
>>>>> Yes. bh_lock_sock/bh_unlock_sock can not block code execution
>>>>> protected by lock_sock/unlock(). That is to say, they are not
>>>>> exclusive.
>>>>>
>>>> No, the logic of the inference is very vague to me. My understand is
>>>> completely different. That is what I read from the kernel code. They
>>>> are not *completely* exclusive, because while the bottom half
>>>> context holds the lock i.e. bh_lock_sock, the process context can
>>>> not get the lock by lock_sock. (This is actually my main point of my
>>>> argument for these fixes, and I didn't see any clarify from you).
>>>> However, while the process context holds the lock by lock_sock, the
>>>> bottom half context can still get it by bh_lock_sock, this is just
>>>> like what you showed in the code in lock_sock. Once it gets the
>>>> ownership, it release the spinlock.
>>>>
>>>
>>> “ while the process context holds the lock by lock_sock, the bottom
>>> half context can still get it by bh_lock_sock,  ”
>>>
>>> You already got that, so why that sock_set_flag(DONE) and
>>> sock_set_flag(DEAD) can not happen concurrently ?
>>>
>>
>> Then I'd ask how do you understand this sentence I wrote? "while the
>> bottom half context holds the lock i.e. bh_lock_sock, the process
>> context can not get the lock by lock_sock."
>>>
>
> That's also true.  I have no questions on it.  They are asymmetrical.
>
> But we cannot guarantee that the interrupt context always holds the lock
> before the process context, that's why i think
> that sock_set_flag(DONE) and sock_set_flag(DEAD) can run concurrently.
>
ok, I have to agree with that. I did too much focus on this case :(
So I think the approach of the 1st patch is also appropriate. Thank you
for taking time to let me out!

>>>>> We can use a very simple example to infer that since bh_lock_sock
>>>>> is type of spin-lock, if bh_lock_sock/bh_unlock_sock can block
>>>>> lock_sock/unlock(),
>>>>> then lock_sock/unlock() can also block bh_lock_sock/bh_unlock_sock.
>>>>>
>>>>> If this is true, when the process context already lock_sock(), the
>>>>> interrupt context must wait for the process to call
>>>>> release_sock(). Obviously, this is very unreasonable.
>>>>>
>>>>>
>>>>>> 2) If the queue_work returns true, as I said in the last main, the
>>>>>> work should be (being) executed. How could the cancel_work_sync()
>>>>>> cancel the work before execution successgully?
>>>>>
>>>>> No, that's not true. In fact, if queue_work returns true, it simply
>>>>> means that we have added the task to the queue and may schedule a
>>>>> worker to execute it,
>>>>> but it does not guarantee that the task will be executed or is
>>>>> being executed when it returns true,
>>>>> the task might still in the list and waiting some worker to execute
>>>>> it.
>>>>>
>>>>> We can make a simple inference,
>>>>>
>>>>> 1. A known fact is that if no special flag (WORK_UNBOUND) is given,
>>>>> tasks submitted will eventually be executed on the CPU where they
>>>>> were submitted.
>>>>>
>>>>> 2. If the queue_work returns true, the work should be or is being
>>>>> executed
>>>>>
>>>>> If all of the above are true, when we invoke queue_work in an
>>>>> interrupt context, does it mean that the submitted task will be
>>>>> executed in the interrupt context?
>>>>>
>>>>>
>>>>> Best wishes,
>>>>> D. Wythe
>>>>>
>>>> If you say the thread is not gauranteed to be waken up in then
>>>> queue_work to execute the work, please explain what the kick_pool
>>>> function does.
>>>
>>> I never said that.
>>>
>> What do you understand on the kick_pool there?
>
>
>
>
> I think this simple logic-code graph can totally explain my point of
> view in clear.
>
> My key point is queue_work can not guarantee the work_1 is executed or
> being executed, the work_1 might still be
> in the list ( before executed ) .
>
> The kick_pool() might wake up the 'a_idle_worker' from schedule(), and
> then the work_1 can be executed soon.
> But we can not said that the  work_1 is already executed or being executed.
>
> In fact, we can invoke cancel_work_syn() to delete the work_1 from the
> list to avoid to be executed, when the
> a_idle_worker_main has not delete(or pop) the work_1 yet.
>
> Besides, there is a upper limit to the number of idle workers. If the
> current number of work_x being executed exceeds this number,
> the work_1 must wait until there are idle_workers available. In that
> case, we can not said that the  work_1 is already executed
> or being executed as well.
>
I do agree with this explaination. My thought was that cancel_work_syn()
deleting the work_1 from the list to avoid to be executed would rarely
happen, as I was focusing the scenario above. Since we have the
agreement on the locks now, I agree that would happen.

Thanks again!
Here you are:
Reviewed-by: Wenjia Zhang <wenjia@linux.ibm.com>