lkml.org 
[lkml]   [2016]   [Dec]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC v2 1/5] rcu: Introduce for_each_leaf_node_cpu()
    On Thu, Dec 15, 2016 at 10:38:58PM +0800, Boqun Feng wrote:
    > On Thu, Dec 15, 2016 at 11:43:52AM +0000, Mark Rutland wrote:
    > > On Thu, Dec 15, 2016 at 10:42:00AM +0800, Boqun Feng wrote:
    > > > +#define MASK_BITS(mask) (BITS_PER_BYTE * sizeof(mask))
    > > > +/*
    > > > + * Iterate over all CPUs a leaf RCU node which are still masked in
    > > > + * @mask.
    > > > + *
    > > > + * Note @rnp has to be a leaf node and @mask has to belong to @rnp.
    > >
    > > Not a big deal, but perhaps it's worth enforcing this? If we took just
    > > the name of the mask here, (e.g. qsmask rather than rnp->qsmask), we
    > > could have the macro always use (rnp)->(mask). That would also make the
    > > invocations shorter.
    >
    > I thought about this approach, but there may be some cases it seems
    > inappropriate, see patch #5, passing "qsmaskinitnext" directly to the
    > for_each_leaf_node_cpu() might be OK, but it just break another
    > abstraction layer which rcu_rnp_online_cpus() provides.

    I had missed that. Given that, not enforcingi t makes sense to me.

    > > > And we
    > > > + * assume that no CPU is masked in @mask but not set in cpu_possible_mask. IOW,
    > > > + * masks of a leaf node never set a bit for an "impossible" CPU.
    > > > + */
    > > > +#define for_each_leaf_node_cpu(rnp, mask, cpu) \
    > > > + for ((cpu) = (rnp)->grplo + find_first_bit(&(mask), MASK_BITS(mask)); \
    > > > + (cpu) <= (rnp)->grphi && !WARN_ON_ONCE(!cpu_possible(cpu)); \
    > >
    > > If this happens, we'll exit the loop. If there are any reamining
    > > possible CPUs, we'll skip them, which would be less than ideal.
    > >
    > > I guess this shouldn't happen anyway, but it might be worth continuing.
    > >
    >
    > I chose to break if we met impossible only because I wanted to avoid
    > using that "if(...) else" trick in an iteration macro ;-)

    Understandable. ;)

    > I don't know whether this is the first time something like this is
    > brought into kernel, so I'm kinda hesitating to bring this in. But seems
    > I got you as one supporter ;-)
    >
    > Certainly, skip is better than stop.

    From a quick look around, I found at least a few instances of the pattern. e.g.

    include/linux/cpufreq.h:

    #define cpufreq_for_each_valid_entry(pos, table) \
    for (pos = table; pos->frequency != CPUFREQ_TABLE_END; pos++) \
    if (pos->frequency == CPUFREQ_ENTRY_INVALID) \
    continue; \
    else

    tools/perf/util/build-id.c:
    #define dsos__for_each_with_build_id(pos, head) \
    list_for_each_entry(pos, head, node) \
    if (!pos->has_build_id) \
    continue; \
    else

    Some drivers, like drivers/net/ethernet/broadcom/bnx2x/bnx2x.h really love it!

    Thanks,
    Mark.

    \
     
     \ /
      Last update: 2016-12-15 16:12    [W:4.409 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site