On Thu, Oct 09, 2014 at 12:50:37PM +0200, Peter Zijlstra wrote:
> On Wed, Oct 01, 2014 at 10:56:35AM +0200, Andrea Arcangeli wrote:
>
> > +static inline long __get_user_pages_locked(struct task_struct *tsk,
> > + struct mm_struct *mm,
> > + unsigned long start,
> > + unsigned long nr_pages,
> > + int write, int force,
> > + struct page **pages,
> > + struct vm_area_struct **vmas,
> > + int *locked,
> > + bool notify_drop)
> > +{
>
> > + if (notify_drop && lock_dropped && *locked) {
> > + /*
> > + * We must let the caller know we temporarily dropped the lock
> > + * and so the critical section protected by it was lost.
> > + */
> > + up_read(&mm->mmap_sem);
> > + *locked = 0;
> > + }
> > + return pages_done;
> > +}
>
> > +long get_user_pages_locked(struct task_struct *tsk, struct mm_struct *mm,
> > + unsigned long start, unsigned long nr_pages,
> > + int write, int force, struct page **pages,
> > + int *locked)
> > +{
> > + return __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
> > + pages, NULL, locked, true);
> > +}
>
> > +long get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm,
> > + unsigned long start, unsigned long nr_pages,
> > + int write, int force, struct page **pages)
> > +{
> > + long ret;
> > + int locked = 1;
> > + down_read(&mm->mmap_sem);
> > + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
> > + pages, NULL, &locked, false);
> > + if (locked)
> > + up_read(&mm->mmap_sem);
> > + return ret;
> > +}
>
> > long get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
> > unsigned long start, unsigned long nr_pages, int write,
> > int force, struct page **pages, struct vm_area_struct **vmas)
> > {
> > + return __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
> > + pages, vmas, NULL, false);
> > }
>
> I'm wondering about that notify_drop parameter, what's the added
> benefit? If you look at these 3 callers we can do away with it, since in
> the second called where we have locked but !notify_drop we seem to do
The second (and third) caller pass notify_drop=false, so the
notify_drop parameter is always a noop for them. They certainly could
get away without it.
> the exact same thing afterwards anyway.
It makes a difference only to the first caller, if it wasn't for the
first caller notify_drop could be dropped. The first caller does this:
return __get_user_pages_locked(tsk, mm, start, nr_pages, write, force,
pages, NULL, locked, true, FOLL_TOUCH);
^ notify_drop = true
Without "notify_drop=true" the first caller could make its own
respective caller think the lock has never been dropped, just because
it is locked by the time get_user_pages_locked returned. But the
caller must be made aware that the lock has been dropped during the
call and in turn any "vma" it got before inside the mmap_sem critical
section is now stale. That's all notify_drop achieves.
Thanks,
Andrea
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
