Date:   Mon, 16 Dec 2019 16:21:46 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Andy Lutomirski' <luto@...nel.org>,
        "Luck, Tony" <tony.luck@...el.com>
CC:     Peter Zijlstra <peterz@...radead.org>,
        "Yu, Fenghua" <fenghua.yu@...el.com>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>
Subject: RE: [PATCH v10 6/6] x86/split_lock: Enable split lock detection by
 kernel parameter

From: Andy Lutomirski
> Sent: 12 December 2019 20:01
> On Thu, Dec 12, 2019 at 11:46 AM Luck, Tony <tony.luck@...el.com> wrote:
> >
> > >> If anything we could switch the entire bitmap interface to unsigned int,
> > >> but I'm not sure that'd actually help much.

That would break all the code that assumes it is 'unsigned long'.
At best it could be changed to a structure with an integral member.
That would make it a little harder for code to 'peek inside' the abstraction.

> > > As we've been looking for potential split lock issues in kernel code, most of
> > > the ones we found relate to callers who have <=32 bits and thus stick:
> > >
> > >       u32 flags;
> > >
> > > in their structure.  So it would solve those places, and fix any future code
> > > where someone does the same thing.

And break all the places that use 'unsigned long' - especially on BE.

> > If different architectures can do better with 8-bit/16-bit/32-bit/64-bit instructions
> > to manipulate bitmaps, then perhaps this is justification to make all the
> > functions operate on "bitmap_t" and have each architecture provide the
> > typedef for their favorite width.

typedef struct { u8/u32/u64 bitmap_val } bitmap_t;

> Hmm.  IMO there are really two different types of uses of the API.
> 
> 1 There's a field somewhere and I want to atomically set a bit.  Something like:
> 
> struct whatever {
>   ...
>   whatever_t field;
>  ...
> };
> 
> struct whatever *w;
> set_bit(3, &w->field);
> 
> If whatever_t is architecture-dependent, then it's really awkward to
> use more than 32 bits, since some architectures won't have more than
> 32-bits.

You could implement that using multiple functions and 'sizeof'.

At the moment that code is broken on BE systems unless whatever_t is
the same size as 'unsigned long'.

> 2. DECLARE_BITMAP(), etc.  That is, someone wants a biggish bitmap
> with a certain number of bits.
> 
> Here the type doesn't really matter.

Except some code uses its own 'unsigned long[]' instead of DECALRE_BITMAP.

The low level x86 code actually passes 'unsigned int[]' knowing that
the cast happened to be ok.

> On an architecture with genuinely atomic bit operations (i.e. no
> hashed spinlocks involved), the width really shouldn't matter.
> set_bit() should promise to be atomic on that bit, to be a full
> barrier, and to not modify adjacent bits.  I don't see why the width
> would matter for most use cases.  If we're concerned, the
> implementation could actually use the largest atomic operation and
> just suitably align it.  IOW, on x86, LOCK BTSQ *where we manually
> align the pointer to 8 bytes and adjust the bit number accordingly*
> should cover every possible case even of PeterZ's concerns are
> correct.

A properly abstracted BITMAP library should be allowed to permute
the bit number using a run-time initialised map.
(eg xor with any value less than the number of bits in 'unsigned long'.)
Otherwise you'll always allow the user to 'peek inside'.

There is also:

1a) I've a field I need to set a bit in.
  There must be a function to do that (I like functions).
  Ah yes:
	set_bit(3, &s->m);
   Bugger doesn't compile, try:
	set_bit(3, (void *)&s->m);
   That must be how I should do it.

   ISTR at least one driver does that when writing ring buffer entries.

2b) I've a 'u32[]', if I cast it to 'unsigned long' I can use the 'bit' functions on it.
   The data never changes (after initialisation), but I've use the atomic operations
   anyway.

> For the "I have a field in a struct and I just want an atomic RMW that
> changes one bit*, an API that matches the rest of the atomic API seems
> nice: just act on atomic_t and atomic64_t.
> 
> The current "unsigned long" thing basically can't be used on a 64-bit
> big-endian architecture with a 32-bit field without gross hackery.

Well, you can xor the bit number with 63 on BE systems.
Then 8/16/32 sized field members work fine - provided you don't
care what values are actually used.

> And sometimes we actually want a 32-bit field.
> 
> Or am I missing some annoying subtlely here?

Some code has assumed DECLARE_BITFIELD() uses 'unsigned long'.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists