Messages in this thread |  | | From | David Laight <> | Subject | RE: [PATCH v10 6/6] x86/split_lock: Enable split lock detection by kernel parameter | Date | Mon, 16 Dec 2019 16:21:46 +0000 |
| |
From: Andy Lutomirski > Sent: 12 December 2019 20:01 > On Thu, Dec 12, 2019 at 11:46 AM Luck, Tony <tony.luck@intel.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)
|  |