Re: [PATCH v10 6/6] x86/split_lock: Enable split lock detection by kernel parameter

[Andy Lutomirski <luto@kernel.org>]

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.
> >
> > 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.
>
> 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.
>

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.


2. DECLARE_BITMAP(), etc.  That is, someone wants a biggish bitmap
with a certain number of bits.

Here the type doesn't really matter.

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.

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.
And sometimes we actually want a 32-bit field.

Or am I missing some annoying subtlely here?