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@xxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
