Re: [PATCH 2/2] mm: Document handle_mm_fault()

[Steven Rostedt]

On Tue, 2011-06-28 at 20:09 +0300, Gleb Natapov wrote:

> > -/*
> > - * By the time we get here, we already hold the mm semaphore
> > +/**
> > + * handle_mm_fault - main routine for handling page faults
> > + * @mm:		the mm_struct of the target address space
> > + * @vma:	vm_area_struct holding the applicable pages
> > + * @address:	the address that took the fault
> > + * @flags:	flags modifying lookup behaviour
> > + *
> > + * Must have @mm->mmap_sem held.
> > + *
> > + * Note: if @flags has FAULT_FLAG_ALLOW_RETRY set then the mmap_sem
> > + *       may be released if it failed to arquire the page_lock. If the
> > + *       mmap_sem is released then it will return VM_FAULT_RETRY set.
> > + *       This is to keep the time mmap_sem is held when the page_lock
> > + *       is taken for IO.
> > + * Exception: If FAULT_FLAG_RETRY_NOWAIT is set, then it will
> > + *       not release the mmap_sem, but will still return VM_FAULT_RETRY
> > + *       if it failed to acquire the page_lock.
> I wouldn't describe it like that. It tells handle_mm_fault() to start
> IO if needed, but do not wait for its completion.

This isn't a description of the flag itself. It's a description of the
mmap_sem behavior for the flag. Again, this came about because of the
subtle locking that handle_mm_fault() does with mmap_sem. When it comes
to locking, we need things documented very well, as locking is usually
what most developers get wrong even when subtle locking does not exist.

> 
> > + *       This is for helping virtualization. See get_user_page_nowait().
> The virtialization is the only user right now, but I wouldn't describe
> this flag as virtialization specific. Why should we put this in the
> comment?  The comment will become outdated when other users arise
> and meanwhile a simple grep will reveal the above information anyway.

I've been going in the direction of adding comments about how things
help. OK, so it's the only user now. I could change this to, "This is
for helping things like virtualization." When reading code, it is nice
to know why something is done that is out of the ordinary.

If it changes, then we should change the comments. The best place for
keeping code documented and up to date, is right at the code. 
ie. comments.


-- Steve


--
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/