Re: [PATCH 2/5] workqueue: merge the similar code

[Lai Jiangshan <laijs@cn.fujitsu.com>]

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