Re: [PATCH 1/4] rmap: add page_wrprotect() function,

[Andrea Arcangeli]

On Tue, Nov 11, 2008 at 01:01:49PM -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2008 21:38:06 +0100
> Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote:
> 
> > > > + * set all the ptes pointed to a page as read only,
> > > > + * odirect_sync is set to 0 in case we cannot protect against race with odirect
> > > > + * return the number of ptes that were set as read only
> > > > + * (ptes that were read only before this function was called are couned as well)
> > > > + */
> > > 
> > > But it isn't.
> > 
> > What isn't?
> 
> This code comment had the kerneldoc marker ("/**") but it isn't a
> kerneldoc comment.

8) never mind, I thought it was referred to the quoted comment, like
if the comment pretended something the function wasn't doing.

> OK, well can we please update the code so these things are clearer.

Sure. Let's say this o_direct fix was done after confirmation that
this was the agreed fix to solve those kind of problems (same fix that
fork will need as in the third link).

> (It's a permanent problem I have.  I ask "what is this", but I really
> mean "the code should be changed so that readers will know what this is")

Agreed, this deserves much more commentary. But not much effort was
done in this area, because fork (and likely migrate) has the same race
and it isn't fixed yet so this is still a work-in-progress area. The
fix has to be the same for all places that have this bug, and we have
not agreed on a way to fix gup_fast yet (all I provided as suggestion
that will work fine is brlock but surely Nick will try to find a way
that remains non-blocking, which is the core of the problem, if it
can't block, we've to use RCU but we can't wait there as far as I can
tell as all sort of synchronous memory regions are involved).

I think once the problem will get fixed and patches will go in
mainline, more docs will emerge (I hope ;). We'll most certainly need
changes to cover gup_fast (including if we use brlock, the read side
of the lock will have to be taken around it). This was a fix we did at
the last minute just to reflect the latest status.

I CC Nick to this thread btw.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/