lkml.org 
[lkml]   [2015]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/5] workqueue: merge the similar code
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


\
 
 \ /
  Last update: 2015-05-12 04:41    [W:0.049 / U:12.744 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site