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

[Wenjia Zhang <wenjia@linux.ibm.com>]

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>