Re: clear_bit_unlock_is_negative_byte

[Matthew Wilcox <willy@infradead.org>]

On Fri, Jul 21, 2023 at 01:29:04PM -0700, Brad Boyer wrote:
> On Fri, Jul 21, 2023 at 12:59:48PM +0100, Matthew Wilcox wrote:
> > On Fri, Jul 21, 2023 at 08:34:55AM +0200, Andreas Schwab wrote:
> > > Why are you using XOR if you want to clear a bit?  If it operates on a
> > > byte, why does it receive a pointer to long?
> > 
> > It's a clever hack.  This function has exactly one user, and it's an
> > important one -- folio_unlock().  Bit 7 is set if there are other
> > threads waiting for this folio to be unlocked.  There are two reasonable
> > implementations, depending what kind of CPU you have; you can either
> > load-locked; clear the bottom bit, store-conditional, test bit 7.  Or
> > x86 and m68k have the perfect instruction to clear a bit and set the
> > Negative flag if bit 7 is set.
> > 
> > As I said in the earlier email, BCLR doesn't affect the N flag, but
> > EORI and ANDI do.  We are guaranteed that the bit we're clearing is set,
> > so EORI will work.  ANDI would also work.
> 
> There shouldn't be any performance difference between using eori.b and
> andi.b, since the compiler would fully generate the immediate mask
> value either way. Wouldn't readability suggest that we should use the
> AND operation to make it more obvious what the code is doing and make
> it more consistent with the other implementations of this function?

That brings me to the next phase of this which is not yet fully baked,
so I wasn't proposing it for discussion at this time.

https://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/folio-flags

What I want to do is allow us to set/clear multiple bits in the same
instruction that unlocks the folio.  The obvious one is
folio_mark_uptodate(); this is commonly paired with folio_unlock(),
and it's a crucial part of page cache reads.  There are some less
obvious ones like folio_start_writeback() and folio_unlock(),
which isn't included in this patch series.  If we're going to set
one bit and clear another bit, we have to use the xor/eor instruction,
and that's what we do.

On some architectures, such as MIPS, there's actually a
separate function to implement all of this and so passing it a
(constant) mask (instead of calculating that mask from what
is now a variable bit) makes sense, and we can then use that
function to implement both clear_bit_unlock_is_negative_byte() and
change_and_unlock_is_negative_byte().  I'm still going around on this.
I might change the API to always pass in a mask from folio_set_unlock().

And maybe we actualy get rid of clear_bit_unlock_is_negative_byte()
since it's essentially a subset of change_and_unlock_is_negative_byte().
But I can change m68k to use andi.b for now if you feel strongly.