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]
Date:   Wed, 10 Apr 2019 04:28:48 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH 4/4] locking/lockdep: Test all incompatible scenario at
 once in check_irq_usage()

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.

Powered by blists - more mailing lists