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