Re: clear_bit_unlock_is_negative_byte

[Michael Schmitz <schmitzmic@gmail.com>]

Hi Brad,

Am 23.07.2023 um 11:49 schrieb Brad Boyer:
> On Sat, Jul 22, 2023 at 04:42:33AM +0100, Matthew Wilcox wrote:
>> 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.
>
> I do prefer to be consistent. We already have strange inconsistencies
> for bad reasons or no reason at all. However, if it's temporary, I
> suppose it doesn't matter that much. With the new code in the diff in
> your git tree, the use of eori is now consistent if you get rid of the
> old clear bit version as separate code. If there's a decent chance it
> will be a while before that next stage, I would personally recommend
> using the andi. However, I would defer to the others on the list if
> people think it's fine.

First off - I know nothing about the code calling this function, so my 
argument may be mooted by design, or proof showing use of xor is safe ...

The only thing that has me worried about the xor scheme is Matthew's 
assertion that the bit meant to be cleared is known to be set before 
clear_bit_unlock_is_negative_byte() is called. Should that be incorrect 
in corner cases, I'd expect things to fall apart with the proposed patch.

Personally, I'd prefer we catch such cases sooner rather than later, 
should they exist. But others may take the view, with rather more 
justification, that the old code using andi is safer and ought to be 
used until other changes force use of xor.

Just my $.02 worth...

Cheers,

	Michael
>
> 	Brad Boyer
> 	flar@allandria.com
>