Added Tejun into CC because of the workqueue API related question at the end of the mail.
On Wed 2021-05-26 19:06:06, Oleg Nesterov wrote:
On 05/24, Petr Mladek wrote:
Your patch changes the semantic. The current semantic is the same for the workqueue's counter-part mod_delayed_work_on().
OK, I see, thanks. I was confused by the comment.
We should actually keep the "ret" value as is to stay compatible with workqueue API:
/* * Canceling could run in parallel from kthread_cancel_delayed_work_sync * and change work's canceling count as the spinlock is released and regain * in __kthread_cancel_work so we need to check the count again. Otherwise, * we might incorrectly queue the dwork and further cause * cancel_delayed_work_sync thread waiting for flush dwork endlessly. * * Keep the ret code. The API primary informs the caller * whether some pending work has been canceled (not proceed). */ if (work->canceling) goto out;
Agreed, we should keep the "ret" value.
Martin Liu, could you please resend the patch without the "ret = false" line? See above.
Andrew, could you please remove this patch from the -mm tree for now?
but unless I am confused again this doesn't match mod_delayed_work_on() which always returns true if it races with cancel(). Nevermind, I think this doesn't matter.
Good point. I think that it is actually a bug. Most callers ignore the return code but there is the following user:
static void addrconf_del_dad_work(struct inet6_ifaddr *ifp) { if (cancel_delayed_work(&ifp->dad_work)) __in6_ifa_put(ifp); } static void addrconf_mod_dad_work(struct inet6_ifaddr *ifp, unsigned long delay) { in6_ifa_hold(ifp); if (mod_delayed_work(addrconf_wq, &ifp->dad_work, delay)) in6_ifa_put(ifp); }
If mod_delayed_work() races with cancel_delayed_work() then both might return true and call in6_ifa_put(ifp).
I thought that they were serialized by ifp->lock. But, for example, addrconf_dad_start() calls addrconf_mod_dad_work() after releasing this lock.
It is possible that they are serialized another way. But I think that in principle only the one that really cancelled a pending work should return "true".
Tejun, any opinion? Feel free to ask for more context.
Best Regards, Petr