Re: [PATCH 3/3 V7] workqueue: Allow modifying low level unbound workqueue cpumask

[Tejun Heo <tj@kernel.org>]

Hello,

Generally looks good to me.  Some minor things below.

On Tue, Apr 07, 2015 at 07:26:37PM +0800, Lai Jiangshan wrote:
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index cbccf5d..557612e 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -299,7 +299,7 @@ static DEFINE_SPINLOCK(wq_mayday_lock);	/* protects wq->maydays list */
>  static LIST_HEAD(workqueues);		/* PR: list of all workqueues */
>  static bool workqueue_freezing;		/* PL: have wqs started freezing? */
>  
> -static cpumask_var_t wq_unbound_global_cpumask;
> +static cpumask_var_t wq_unbound_global_cpumask; /* PL: low level cpumask for all unbound wqs */

Are we set on this variable name?  What would we lose by naming it
wq_unbound_cpumask or wq_cpu_possible_mask?

> @@ -3493,6 +3493,7 @@ static struct pool_workqueue *numa_pwq_tbl_install(struct workqueue_struct *wq,
>  struct apply_wqattrs_ctx {
>  	struct workqueue_struct	*wq;	/* target to be applied */
>  	struct workqueue_attrs	*attrs;	/* configured attrs */
> +	struct list_head	list;	/* queued for batching commit */
                                                      batch commit
>  	struct pool_workqueue	*dfl_pwq;
>  	struct pool_workqueue	*pwq_tbl[];
>  };
> @@ -3517,7 +3518,8 @@ static void apply_wqattrs_cleanup(struct apply_wqattrs_ctx *ctx)
>  /* Allocates the attrs and pwqs for later installment */
>  static struct apply_wqattrs_ctx *
>  apply_wqattrs_prepare(struct workqueue_struct *wq,
> -		      const struct workqueue_attrs *attrs)
> +		      const struct workqueue_attrs *attrs,
> +		      cpumask_var_t unbound_cpumask)

Why do we need this tho?  The global mask is protected by pool mutex,
right?  The update function can set it to the new value and just call
update and revert on failure.

> @@ -3710,6 +3721,14 @@ static void wq_update_unbound_numa(struct workqueue_struct *wq, int cpu,
>  	 * wq's, the default pwq should be used.
>  	 */
>  	if (wq_calc_node_cpumask(wq->unbound_attrs, node, cpu_off, cpumask)) {
> +		/*
> +		 * wq->unbound_attrs is the user configured attrs whose
> +		 * cpumask is not masked with wq_unbound_global_cpumask,
> +		 * so we make complete it.
> +		 */
> +		cpumask_and(cpumask, cpumask, wq_unbound_global_cpumask);
> +		if (cpumask_empty(cpumask))
> +			goto use_dfl_pwq;

Wouldn't it be better to apply the global cpumask before calling
wq_calc_node_cpumask()?  Or just move it inside wq_calc_node_cpumask?

Thanks.

-- 
tejun