linux-kernel - Re: [RFC v2 1/5] rcu: Introduce for_each_leaf_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20161215151416.GO9728@tardis.cn.ibm.com>
Date:   Thu, 15 Dec 2016 23:14:16 +0800
From:   Boqun Feng <boqun.feng@...il.com>
To:     Mark Rutland <mark.rutland@....com>
Cc:     linux-kernel@...r.kernel.org,
        "Paul E . McKenney " <paulmck@...ux.vnet.ibm.com>,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Colin King <colin.king@...onical.com>
Subject: Re: [RFC v2 1/5] rcu: Introduce for_each_leaf_node_cpu()

On Thu, Dec 15, 2016 at 03:10:15PM +0000, Mark Rutland wrote:
> 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!
> 

Thank you!
So no reason we don't use it, let me send an updated patch ;-)

Regards,
Boqun

> Thanks,
> Mark.

Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)