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

[Frederic Weisbecker]

On Thu, Apr 11, 2019 at 12:46:32PM +0200, Peter Zijlstra wrote:
> On Wed, Apr 10, 2019 at 04:28:48AM +0200, Frederic Weisbecker wrote:
> > Should I re-issue the set or you do the changes?
> 
> I've made it like so; does that work? In particular, do the comments
> make sense? It is always hard to write these things down.

It's especially hard because we describe both the bits of the usage mask
and the bits of the bit numbers of the usage mask. The resulting description
can only be naughty :-)

That said I can probably clarify that in lockdep_internals.h on further
patches.

A few comments though:

> +/*
> + * The bit number is encoded like:
> + *
> + *  bit0: 0 exclusive, 1 read lock
> + *  bit1: 0 used in irq, 1 irq enabled
> + *  bit2-n: state
> + */
>  static int exclusive_bit(int new_bit)
>  {
>  	int state = new_bit & LOCK_USAGE_STATE_MASK;
> @@ -1988,45 +1968,158 @@ static int exclusive_bit(int new_bit)
>  	return state | (dir ^ LOCK_USAGE_DIR_MASK);
>  }
>  
> +/*
> + * Observe that when given a bitmask where each bitnr is encoded as above, a
> + * right shift of the mask transforms the individual bitnrs as -1.
> + *
> + * So for all bits where bitnr1 == 1, we can create the mask where bitnr1 == 0

So by bitnr1 you're referring to direction, right?

> + * by subtracting 2, or shifting the mask right by 2.

In which case we can perhaps reformulate:

  So for all bits whose number have LOCK_ENABLED_* set (bitnr1 == 1), we can create the
  mask with those bit numbers using LOCK_USED_IN_* (bitnr1 == 0) instead by
  subtracting the bit number by 2, or shifting the mask right by 2.

And same would go for below.

> + *
> + * Similarly, bitnr1 == 0 becomes bitnr1 == 1 by adding 2, or shifting left 2.
> + *
> + * So split the mask (note that LOCKF_ENABLED_IRQ_ALL|LOCKF_USED_IN_IRQ_ALL is
> + * all bits set) and recompose with bitnr1 flipped.
> + */
> +static unsigned long invert_dir_mask(unsigned long mask)
> +{
> +	unsigned long excl = 0;
> +
> +	/* 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;
> +}
> +
> +/*
> + * As above, we clear bitnr0 with bitmask ops. First, for all bits with bitnr1
> + * set, add those with bitnr1 cleared. And then mask out all bitnr1.
> + */

Same here:

  As above, we clear bitnr0 (LOCK_*_READ off) with bitmask ops. First, for all bits
  with bitnr1 set (LOCK_ENABLED_*) , add those with bitnr1 cleared (LOCK_USED_IN_*).
  And then mask out all bitnr1.

> +static unsigned long exclusive_mask(unsigned long mask)
> +{
> +	unsigned long excl = invert_dir_mask(mask);
> +
> +	/* Strip read */
> +	excl |= (excl & LOCKF_IRQ_READ) >> LOCK_USAGE_READ_MASK;
> +	excl &= ~LOCKF_IRQ_READ;
> +
> +	return excl;
> +}

Not sure I'm making things clearer but at least that's some more pointers
to enum lock_usage_bit (defined on headers where I should add more layout
explanations, especially to make it clear we play with bit number bits :-s  )

Thanks.