Re: [PATCH 4/4] locking/lockdep: Test all incompatible scenario at once in check_irq_usage()

[Frederic Weisbecker]

On Tue, Apr 09, 2019 at 03:03:52PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 02, 2019 at 06:02:44PM +0200, Frederic Weisbecker wrote:
> > @@ -1988,45 +1961,151 @@ static int exclusive_bit(int new_bit)
> >  	return state | (dir ^ LOCK_USAGE_DIR_MASK);
> >  }
> >  
> > +static unsigned long exclusive_dir_mask(unsigned long mask)
> 
> Would you mind terribly if I call that: invert_dir_mask() ?

That's indeed much better.

> 
> > +{
> > +	unsigned long excl;
> > +
> > +	/* Invert dir */
> > +	excl = (mask & LOCKF_ENABLED_IRQ_ALL) >> LOCK_USAGE_DIR_MASK;
> > +	excl |= (mask & LOCKF_USED_IN_IRQ_ALL) << LOCK_USAGE_DIR_MASK;
> > +
> > +	return excl;
> > +}
> > +
> > +static unsigned long exclusive_mask(unsigned long mask)
> > +{
> > +	unsigned long excl = exclusive_dir_mask(mask);
> > +
> > +	/* Strip read */
> > +	excl |= (excl & LOCKF_IRQ_READ) >> LOCK_USAGE_READ_MASK;
> > +	excl &= ~LOCKF_IRQ_READ;
> > +
> > +	return excl;
> > +}
> 
> And I might write a comment to go with those functions; they're too
> clever by half. I'm sure I'll have forgotten how they work in a few
> months time.

It only takes a night for me to forget how my code works. Then I need
a whole day long to recollect. But once I'm done the next night starts.

So I'm not against comments, thanks :-)

> > +/*
> > + * Find the first pair of bit match between an original
> > + * usage mask and an exclusive usage mask.
> > + */
> > +static int find_exclusive_match(unsigned long mask,
> > +				unsigned long excl_mask,
> > +				enum lock_usage_bit *bit,
> > +				enum lock_usage_bit *excl_bit)
> > +{
> > +	int fs, nr = 0;
> > +
> > +	while ((fs = ffs(mask))) {
> > +		int excl;
> > +
> > +		nr += fs;
> > +		excl = exclusive_bit(nr - 1);
> > +		if (excl_mask & lock_flag(excl)) {
> > +			*bit = nr - 1;
> > +			*excl_bit = excl;
> > +			return 0;
> > +		}
> > +		mask >>= fs - 1;
> > +		/*
> > +		 * Prevent from shifts of sizeof(long) which can
> > +		 * give unpredictable results.
> > +		 */
> > +		mask >>= 1;
> > +	}
> > +	return -1;
> 
> Should we write that like:
> 
> 	for_each_set_bit(bit, &mask, LOCK_USED) {
> 		int excl = exclusive_bit(bit);
> 		if (excl_mask & lock_flag(excl)) {
> 			*bitp = bit;
> 			*excl_bitp = excl;
> 			return 0;
> 		}
> 	}
> 	return -1;
> 
> Or something along those lines?

Ah yes indeed. Linus didn't like the idea of using bitmap functions for lockdep
usage bits in the softirq vector patchset but here the case is different as it's
not used in lockdep fastpath. That's only for the bad locking scenario path so it's
all good I think.

Should I re-issue the set or you do the changes?

Thanks.