Re: [PATCH 05/10] nohz: New tick dependency mask

[Frederic Weisbecker <fweisbec@gmail.com>]

On Mon, Aug 03, 2015 at 03:24:07PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 03, 2015 at 03:05:37PM +0200, Frederic Weisbecker wrote:
> > On Mon, Aug 03, 2015 at 02:43:50PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 23, 2015 at 06:42:10PM +0200, Frederic Weisbecker wrote:
> > > > +unsigned long __tick_nohz_set_tick_dependency(enum tick_dependency_bit bit,
> > > > +					      unsigned long *dep)
> > > > +{
> > > > +	unsigned long prev;
> > > > +	unsigned long old = *dep;
> > > > +	unsigned long mask = BIT_MASK(bit);
> > > > +
> > > > +	while ((prev = cmpxchg(dep, old, old | mask)) != old) {
> > > > +		old = prev;
> > > > +		cpu_relax();
> > > > +	}
> > > > +
> > > > +	return prev;
> > > > +}
> > > 
> > > That's typically called a fetch_or(). The function name, which is
> > > entirely too long, also doesn't suggest a return value.
> > 
> > Nice, I can export fetch_or() from sched code to atomic_fetch_or() like
> > you just suggested and bring it to asm_generic if you like.
> 
> asm_generic means you _have_ to go implement it for all archs that do
> not use asm_generic.
> 
> The version I have is also unsuited for the atomic name, as it deals
> with all integer types that cmpxchg() can consume (and I need it that
> way, because thread_info::flags does not have a consistent type across
> archs (unsigned long vs unsigned int is the most common variation).

Indeed, and I need clear_bit() to be the symetric call and that doesn't
seem to operate on atomic_t.

> 
> You can go add it to linux/atomic.h I suppose, with the regular #ifdef
> guards we have there. Then add:
> 
>   atomic{,long,64}_fetch_{add,sub,inc,dec,or,and,xor}()
> 
> I suppose, using cmpxchg() loops and then let archs implement as we go.

Right.

> 
> > So perhaps fetch_and_or() ? Of course having "and" then "or" in the same function name
> > might be confusing but after all we have functions names starting with "test" then "and".
> 
> No fetch_$op is a well known name for these things, don't go change it.
> That just means people will _not_ find it.

Ok.

> 
> > Concerning the nohz function name, tick_nohz is a long prefix that is very likely
> > to produce too long names. Perhaps I should trim tick_nohz to just nohz.
> 
> No, then we'll get confused with the regular nohz stuff. But yes a 10
> letter prefix sucks, but at least you can try and not then add another
> 30.

Well regular nohz stuff (that is, idle nohz right?) and nohz full have the same prefix.
We just have tick_nohz_full_* for very specific things but that's still compatible with
a common reduced prefix.

But lets keep it that way for now, if a rename is needed, we'll do it beyond the scope
of that set. I'll just take your tick_nohz_set_dep() suggestion.

Thanks.