lkml.org 
[lkml]   [2008]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] cpumask: introduce new API, without changing anything

    * Ingo Molnar <mingo@elte.hu> wrote:

    > but the change was not due to some merge conflict, it was a change
    > done to the patch itself.
    >
    > The merge conflict happened because Rusty iterated the patch in a
    > non-append manner so two versions of the same patch collided in
    > linux-next.
    >
    > So ... what was the change, was it _really_ tested as-is in the
    > linux-next tree for a longer time, or just merged a couple of hours
    > ago?

    hm, i just researched it a bit, and the patch Rusty just submitted to
    Linus was not in linux-next it appears, as the modified commit is just
    a few hours old.

    Find below the delta patch between the two versions, and an
    analysis/review of the changes. I've started testing it as well, and
    it's looking good so far.

    Here is when the new version showed up in linux-next a few hours ago:

    | commit 92a8334f1b0ad1f65127bc763ab214d6b60ec966
    | Author: Stephen Rothwell <sfr@canb.auug.org.au>
    | AuthorDate: Fri Nov 7 20:44:35 2008 +1100
    | Commit: Stephen Rothwell <sfr@canb.auug.org.au>
    | CommitDate: Fri Nov 7 20:44:35 2008 +1100
    |
    | Add linux-next specific files for 20081107

    It was a new (and modified) quilt-exported commit from "quilt/rr":

    | 0c82f41: cpumask:new-API-only
    |
    | Author: Rusty Russell <rusty@rustcorp.com.au>
    | AuthorDate: Fri Nov 7 11:12:29 2008 +1100
    | Commit: Stephen Rothwell <sfr@canb.auug.org.au>
    | CommitDate: Fri Nov 7 11:12:29 2008 +1100

    Which collided with a previous version of that patch in the cpus4096
    tree:

    | 2d3854a: cpumask: introduce new API, without changing anything
    |
    | Author: Rusty Russell <rusty@rustcorp.com.au>
    | AuthorDate: Wed Nov 5 13:39:10 2008 +1100
    | CommitDate: Thu Nov 6 09:05:33 2008 +0100

    "0c82f41: cpumask:new-API-only" thus has only a couple of hours of
    lifetime and the "this has been in linux-next" is only true for that
    short time period.

    So ... i've reconstructed what's new in 0c82f41, and i came up with
    the delta patch below.

    Review of the delta patch:

    - the cpumask_of() is added to the SMP section of cpumask.h but not to
    the UP section. Similarly, some of the other SMP APIs are not
    declared in the UP section either. (Furthermore, the macros in the
    UP section should really be inlines, not macros - as akpm noticed it
    too.)

    - the free_bootmem_cpumask_var() addition looks good but is a tiny bit
    incomplete: the free_bootmem_cpumask_var() function should be marked
    __init, like all bootmem methods are.

    i guess these changes should be OK.

    Note that such types of minor problems are very easy to spot and
    address in an append-only workflow (even if you just emulate it in
    quilt), but are hidden and laborous to extract with an agreesive
    quilt-rebasing history-eating workflow that just exports completely
    new versions of patches.

    Just in case you are wondering why i care so much about 'your' patch:
    i'd like it to go upstream. The patch and the plan affects subsystems
    i'm (co-)maintaining, in a nontrivial way, (scheduler, irq and x86
    code), and the cpumask code caused trouble in the past so i'd like to
    know _exactly_ what future changes go into it. Isnt that a fair
    interest?

    But your "rr tree" is not suitable at all to follow the development of
    this effort. Deltas are not transparent and reviewable, the changes
    are mixed into other changes and you force me into this kind of review
    and delta patch forensics and that is very inefficient - and it
    obviously harms the end result and wastes time for no good reason.

    And yes, when all is said and done and everyone's happy there's no
    problem with eventually collapsing delta patches together into a
    single clean patch.

    Most of the time it's not _worth_ touching existing history with
    folding patches: as you can see it from the patch below, there's no
    technical reason why it could not have been a separate delta patch.
    Those changes do not cause _any_ bisection barrier, nor do they cause
    any other of problem either.

    And dont get me wrong, destroying the delta was not malice on your
    part in any way, it was "just" a side-effect destruction of
    information that resulted out of your customary work flow. I realize
    that you like and prefer that workflow, and that you have used it for
    years, but you should also accept it when i point out the problems
    that it causes down the line.

    And the thing is, a year ago i was in similar shoes and i was on the
    receiving end of similar complaints, and in hindsight i do not regret
    at all having changed to a Git workflow =B-)

    Ingo

    ----------------->
    From cd83e42c6b0413dcbb548c2ead799111ff7e6a13 Mon Sep 17 00:00:00 2001
    From: Rusty Russell <rusty@rustcorp.com.au>
    Date: Fri, 7 Nov 2008 11:12:29 +1100
    Subject: [PATCH] cpumask: new API, v2

    - add cpumask_of()
    - add free_bootmem_cpumask_var()

    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    ---
    include/linux/cpumask.h | 11 +++++++++++
    lib/cpumask.c | 5 +++++
    2 files changed, 16 insertions(+), 0 deletions(-)

    diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
    index c8e6661..31caa1b 100644
    --- a/include/linux/cpumask.h
    +++ b/include/linux/cpumask.h
    @@ -894,6 +894,12 @@ static inline void cpumask_copy(struct cpumask *dstp,
    #define cpumask_any_and(mask1, mask2) cpumask_first_and((mask1), (mask2))

    /**
    + * cpumask_of - the cpumask containing just a given cpu
    + * @cpu: the cpu (<= nr_cpu_ids)
    + */
    +#define cpumask_of(cpu) (get_cpu_mask(cpu))
    +
    +/**
    * to_cpumask - convert an NR_CPUS bitmap to a struct cpumask *
    * @bitmap: the bitmap
    *
    @@ -946,6 +952,7 @@ typedef struct cpumask *cpumask_var_t;
    bool alloc_cpumask_var(cpumask_var_t *mask, gfp_t flags);
    void alloc_bootmem_cpumask_var(cpumask_var_t *mask);
    void free_cpumask_var(cpumask_var_t mask);
    +void free_bootmem_cpumask_var(cpumask_var_t mask);

    #else
    typedef struct cpumask cpumask_var_t[1];
    @@ -962,6 +969,10 @@ static inline void alloc_bootmem_cpumask_var(cpumask_var_t *mask)
    static inline void free_cpumask_var(cpumask_var_t mask)
    {
    }
    +
    +static inline void free_bootmem_cpumask_var(cpumask_var_t mask)
    +{
    +}
    #endif /* CONFIG_CPUMASK_OFFSTACK */

    /* The pointer versions of the maps, these will become the primary versions. */
    diff --git a/lib/cpumask.c b/lib/cpumask.c
    index 5ceb421..2ebc3a9 100644
    --- a/lib/cpumask.c
    +++ b/lib/cpumask.c
    @@ -107,4 +107,9 @@ void free_cpumask_var(cpumask_var_t mask)
    kfree(mask);
    }
    EXPORT_SYMBOL(free_cpumask_var);
    +
    +void free_bootmem_cpumask_var(cpumask_var_t mask)
    +{
    + free_bootmem((unsigned long)mask, cpumask_size());
    +}
    #endif

    \
     
     \ /
      Last update: 2008-11-07 13:27    [W:2.863 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site