Re: [PATCH v2 16/31] workqueue: introduce workqueue_attrs

[Ryan Mallon <rmallon@gmail.com>]

On 06/03/13 09:44, Tejun Heo wrote:
> Hello,
> 
> On Wed, Mar 06, 2013 at 09:40:48AM +1100, Ryan Mallon wrote:
>>> Ooh, right, and that cpumask_t is going away and you can't statically
>>> allocate cpumask_var_t, so it needs an allocation and error check from
>>> it anyway.
>>
>> Not sure I follow. I mean drop the pointer, eg:
>>
>> 	struct workqueue_attr attrs;
>>
>> Since, at least in this patch, struct worker_pool appears to always
>> alloc the attrs field. You do still of course need the cpumask_t
>> initialisation. Am I missing something?
> 
> So, new usages of cpumask_t is frowned upon and we gotta use
> cpumask_var_t which needs alloc_cpumask_var() which may fail, so we
> have try-to-alloc-and-check-for-failure no matter what.  Now, if we
> want to embed workqueue_attrs, we have to separate out initialization
> of allocated attrs from the actaul allocation.  ie. we'll need
> init_workqueue_attrs() and alloc_workqueue_attrs() and as the former
> may fail too, it doesn't really simplify pool initilaization path.
> So, we end up with more code.  The added code is minor but it also
> doesn't buy anything.

I don't get why you would need to separate init/alloc. Nothing in the
patch series appears to have optional attrs (e.g. a case where attrs
might be NULL), so allocing isn't necessary, which is my point. The init
function can fail due to the cpumask_t, as you point out, but at least
you can remove one alloc/free per attrs struct:

static int workqueue_init_attrs(struct workqueue_attrs *attrs,
				gfp_t gfp_mask)
{
	memset(attrs, 0, sizeof(*attrs));
	if (!alloc_cpumask_var(&attrs->cpumask, gfp_mask))
		return -ENOMEM;
	cpumask_setall(attrs->cpumask);
	return 0;
}

static void workqueue_deinit_attrs(struct workqueue_attrs *attrs)
{
	free_cpumask_var(attrs->cpumask);
}

In patch 17 unbound_std_wq_attrs can easily be changed to a non-pointer
type, and in patch 31 you remove the need to alloc/free the attrs
structure in wq_nice_store, so you would have something like:

	struct workqueue_attrs attrs;
	int err;

	err = workqueue_init_attrs(&attrs, GFP_KERNEL);
	if (err)
		return err;

	rcu_read_lock_sched();
	copy_workqueue_attrs(&attrs, first_pwq(wq)->pool->attrs);
	rcu_read_unlock_sched();

	apply_workqueue_attrs(wq, &attrs);

	/* Needed to free the temp cpumask */
	workqueue_deinit_attrs(&attrs);

If there are cases where the attrs need to be a pointer (e.g. it can
optionally be NULL, which needs to be tested against), then you could
just leave the responsibility of allocation to the caller.

~Ryan