Messages in this thread | | | Date | Wed, 06 Mar 2013 10:20:20 +1100 | From | Ryan Mallon <> | Subject | Re: [PATCH v2 16/31] workqueue: introduce workqueue_attrs |
| |
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
| |