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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150422193935.GG10738@htj.duckdns.org>
Date:	Wed, 22 Apr 2015 15:39:35 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	linux-kernel@...r.kernel.org, Christoph Lameter <cl@...ux.com>,
	Kevin Hilman <khilman@...aro.org>,
	Mike Galbraith <bitbucket@...ine.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH 3/3 V7] workqueue: Allow modifying low level unbound
 workqueue cpumask

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/