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:2.578 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site