On 05/11/2015 10:31 PM, Tejun Heo wrote:
> Hello, Lai.
Hello, TJ
>
>> * @node: the target NUMA node
>> - * @cpu_going_down: if >= 0, the CPU to consider as offline
>> - * @cpumask: outarg, the resulting cpumask
>> + * @cpu_off: if >= 0, the CPU to consider as offline
>
> @cpu_off sounds like offset into cpu array or sth. Is there a reason
> to change the name?
@cpu_off is a local variable in wq_update_unbound_numa() and is a shorter
name.
>
>> + *
>> + * Allocate or reuse a pwq with the cpumask that @wq should use on @node.
>
> I wonder whether a better name for the function would be sth like
> get_alloc_node_unbound_pwq().
>
The name length of alloc_node_unbound_pwq() had already added trouble to me
for code-indent in the called-site. I can add a variable to ease the indent
problem later, but IMHO, get_alloc_node_unbound_pwq() is not strictly a better
name over alloc_node_unbound_pwq(). Maybe we can consider
get_node_unbound_pwq()?
>> *
>> - * Calculate the cpumask a workqueue with @attrs should use on @node. If
>> - * @cpu_going_down is >= 0, that cpu is considered offline during
>> - * calculation. The result is stored in @cpumask.
>> + * If NUMA affinity is not enabled, @dfl_pwq is always used. @dfl_pwq
>> + * was allocated with the effetive attrs saved in @dfl_pwq->pool->attrs.
>
> I'm not sure we need the second sentence.
effetive -> effective
I used "the effetive attrs" twice bellow. I need help to rephrase it,
might you do me a favor? Or just use it without introducing it at first?
+ * If enabled and @node has online CPUs requested by the effetive attrs,
+ * the cpumask is the intersection of the possible CPUs of @node and
+ * the cpumask of the effetive attrs.
>> + if (cpumask_equal(cpumask, attrs->cpumask))
>> + goto use_dfl;
>> + if (pwq && wqattrs_equal(tmp_attrs, pwq->pool->attrs))
>> + goto use_existed;
>
> goto use_current;
The label use_existed is shared with use_dfl:
use_dfl:
pwq = dfl_pwq;
use_existed:
spin_lock_irq(&pwq->pool->lock);
get_pwq(pwq);
spin_unlock_irq(&pwq->pool->lock);
return pwq;
But I don't think the dfl_pwq is current.
>
> Also, would it be difficult to put this in a separate patch? This is
> mixing code refactoring with behavior change. Make both code paths
> behave the same way first and then refactor?
>
>> +
>> + /* create a new pwq */
>> + pwq = alloc_unbound_pwq(wq, tmp_attrs);
>> + if (!pwq && use_dfl_when_fail) {
>> + pr_warn("workqueue: allocation failed while updating NUMA
>> affinity of \"%s\"\n",
>> + wq->name);
>> + goto use_dfl;
>
> Does this need to be in this function? Can't we let the caller handle
> the fallback instead?
Will it leave the duplicated code that this patch tries to remove?
I will try it with introducing a get_pwq_unlocked().
Thanks,
Lai
--
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/
