Messages in this thread | | | Date | Tue, 12 May 2015 10:15:28 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH 4/5] workqueue: don't expose workqueue_attrs to users |
| |
On 05/11/2015 10:59 PM, Tejun Heo wrote: > On Mon, May 11, 2015 at 05:35:51PM +0800, Lai Jiangshan wrote: >> workqueue_attrs is an internal-like structure and is exposed with >> apply_workqueue_attrs() whose user has to investigate the structure >> before use. >> >> And the apply_workqueue_attrs() API is inconvenient with the structure. >> The user (although there is no user yet currently) has to assemble >> several LoC to use: >> attrs = alloc_workqueue_attrs(); >> if (!attrs) >> return; >> attrs->nice = ...; >> copy cpumask; >> attrs->no_numa = ...; >> apply_workqueue_attrs(); >> free_workqueue_attrs(); >> >> It is too elaborate. This patch changes apply_workqueue_attrs() API, >> and one-line-code is enough to be called from user: >> apply_workqueue_attrs(wq, cpumask, nice, numa); >> >> This patch also reduces the code of workqueue.c, about -50 lines. >> wq_sysfs_prep_attrs() is removed, wq_[nice|cpumask|numa]_store() >> directly access to the ->unbound_attrs with the protection >> of apply_wqattrs_lock(); >> >> This patch is also a preparation patch of next patch which >> remove no_numa out from the structure workqueue_attrs which >> requires apply_workqueue_attrs() has an argument to pass numa affinity. > > I'm not sure about this. Yeah, sure, it's a bit more lines of code > but at the same time this'd allow us to make the public interface > atomic too. What we prolly should do is changing the interface so > that we do > > attrs = prepare_workqueue_attrs(gfp_mask); /* allocate, lock & copy */ > /* modify attrs as desired */ > commit_workqueue_attrs(attrs); /* apply, unlock and free */
I think the workqueue.c has too much complicated and rarely used APIs and exposes too much in this way. No one can set the nice value and the cpuallowed of a task atomically.
If the user want atomic-able, Her/he can just disable WQ_SYSFS on its workqueue and maintain a copy of the cpumask, nice, numa values under its own lock.
> > Thanks. >
| |