Re: VM-related Oops: 2.4.15pre1

[Linus Torvalds]

On 19 Nov 2001, Eric W. Biederman wrote:
>
> However this case seems to violate code clarity.  If you can have
> other users testing PG_locked it is non-intuitive that you can still
> normally assign to page->flags.

This is 100% true, and I actually want to fix it.

The ugliness is actually a historical remnant - we do it with a simple
store simply because at the time we add a page to the page cache, we
historically were the only users of that page. Nobody else could get
access to it, because it didn't exist on any lists.

These days, the LRU queue exists before the page is added to the swap
cache, which means that other people actually _can_ see the page, and the
historical "initialize page flags" is no longer touching a purely private
field.

> Would it make sense to add a set_bits macro that is a just an
> assignment except on extremely weird architectures or to work
> around compiler bugs?  I'm just thinking it would make sense
> to document that we depend on the compiler not writing some
> strange intermediate values into the variable.

The thing is, "__add_to_page_cache()" really shouldn't touch the page
flags AT ALL, not with a "set_bits()" macro, and not with anything else
either for that matter. It's actually clearing flags that have meaning,
and the callers these days have to undo some of the work that it does (ie
see how try_to_swap_out() ends up having to mark the page dirty again,
only because __add_to_page_cache() cleared the dirty bit that it shouldn't
know anything at all about).

So the real fix for this particular case is to move the bit
setting/clearing into those callers that actually _want_ to set the bits,
some of which actually do have exclusive access to the page.

That doesn't change the fact that other parts of the kernel still assume
that the setting of a pointer or status field is atomic, for example, and
then use the memory barrier macros to force memory ordering (both for gcc
and for the CPU at runtime).

			Linus