Messages in this thread | | | From | Lai Jiangshan <> | Date | Wed, 7 Mar 2018 10:49:49 +0800 | Subject | Re: [PATCH 7/7] RCU, workqueue: Implement rcu_work |
| |
On Wed, Mar 7, 2018 at 1:33 AM, Tejun Heo <tj@kernel.org> wrote:
> +/** > + * queue_rcu_work_on - queue work on specific CPU after a RCU grace period > + * @cpu: CPU number to execute work on > + * @wq: workqueue to use > + * @rwork: work to queue
For many people, "RCU grace period" is clear enough, but not ALL.
So please make it a little more clear that it just queues work after a *Normal* RCU grace period. it supports only one RCU variant.
> + * > + * Return: %false if @work was already on a queue, %true otherwise. > + */
I'm afraid this will be a hard-using API.
The user can't find a plan B when it returns false, especially when the user expects the work must be called at least once again after an RCU grace period.
And the error-prone part of it is that, like other queue_work() functions, the return value of it is often ignored and makes the problem worse.
So, since workqueue.c provides this API, it should handle this problem. For example, by calling call_rcu() again in this case, but everything will be much more complex: a synchronization is needed for "calling call_rcu() again" and allowing the work item called twice after the last queue_rcu_work() is not workqueue style.
Some would argue that the delayed_work has the same problem when the user expects the work must be called at least once again after a period of time. But time interval is easy to detect, the user can check the time and call the queue_delayed_work() again when needed which is also a frequent design pattern. And for rcu, it is hard to use this design pattern since it is hard to detect (new) rcu grace period without using call_rcu().
I would not provide this API. it is not a NACK. I'm just trying expressing my thinking about the API. I'd rather RCU be changed and RCU callbacks are changed to be sleepable. But a complete overhaul cleanup on the whole source tree for compatibility is needed at first, an even more complex job.
> +bool queue_rcu_work_on(int cpu, struct workqueue_struct *wq, > + struct rcu_work *rwork) > +{ > + struct work_struct *work = &rwork->work; > + > + if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) { > + rwork->wq = wq; > + rwork->cpu = cpu; > + call_rcu(&rwork->rcu, rcu_work_rcufn); > + return true; > + } > + > + return false; > +} > +EXPORT_SYMBOL(queue_rcu_work_on); > +
| |