lkml.org 
[lkml]   [2009]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] workqueue: don't alloc_percpu for single workqueue
    Hi, Oleg Nesterov

    The new version of this patch has not been accepted.
    I think it's due to that I forgot reply this mail.

    (comment nesting)

    Oleg Nesterov wrote:
    > On 01/22, Lai Jiangshan wrote:
    >> Oleg Nesterov wrote:
    >>> On 01/21, Lai Jiangshan wrote:
    >>>> @@ -906,6 +907,13 @@ void destroy_workqueue(struct workqueue_struct *wq)
    >>>> const struct cpumask *cpu_map = wq_cpu_map(wq);
    >>>> int cpu;
    >>>>
    >>>> + if (is_wq_single_threaded(wq)) {
    >>>> + cleanup_workqueue_thread(wq->cpu_wq);
    >>>> + kfree(wq->cpu_wq);
    >>>> + kfree(wq);
    >>>> + return;
    >>>> + }
    >>> again, not sure I understand why this change is needed. Afaics we
    >>> only need to use kfree(wq->cpu_wq) instead of free_percpu() if
    >>> it is single-threaded.
    >>>
    >> I think this change is needed.
    >> In the single thread case, we don't need
    >> 1) cpu_maps_update_begin(). --> require cpu_add_remove_lock
    >> 2) remove workqueue from the list. (we did not inserted it)
    >>
    >> It is indeed that there is no bad result occurred when we do these
    >> things for single thread. But I think the destroying should not
    >> do things more than the creating.
    >
    > I disagree.
    >
    > Firstly, this path is rare and not time critical, it is better
    > to save a couple of bytes from .text.

    For non-critical path, I think the prior sequence aim is:

    code logic is right
    code's readability
    save .text size
    save cpu cycle

    I want to make "code logic is right", so
    my patch adds another special case for single thread workqueue.

    And in creating site, single thread workqueue are also considered
    special.

    Thanks
    Lai.

    >
    > But mostly I dislike the fact that we add another special case
    > for the single-threaded wqs which is not strictly needed.
    >
    > Following your logic we can also change flush_workqueue(), it
    > doesn't need for_each_cpu_mask_nr() when single-threaded.
    >
    >
    > That said, I agree this is a matter of taste, I won't persist.
    >
    > Oleg.
    >



    \
     
     \ /
      Last update: 2009-02-18 12:23    [W:0.025 / U:0.556 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site