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.
