Re: [RFC][PATCH] Clarify mem_cgroup lock handling and avoid races.

[Hugh Dickins <hugh@veritas.com>]

On Wed, 20 Feb 2008, Balbir Singh wrote:
> Hugh Dickins wrote:
> > On Wed, 20 Feb 2008, Balbir Singh wrote:
> >>> -	if (pc)
> >>> -		VM_BUG_ON(!page_cgroup_locked(page));
> >>> -	locked = (page->page_cgroup & PAGE_CGROUP_LOCK);
> >>> -	page->page_cgroup = ((unsigned long)pc | locked);
> >>> +	page->page_cgroup = ((unsigned long)pc | PAGE_CGROUP_LOCK);
> >> We are explicitly setting the PAGE_CGROUP_LOCK bit, shouldn't we keep the
> >> VM_BUG_ON(!page_cgroup_locked(page))?
> > 
> > Could do, but it seemed quite unnecessary to me now that it's a static
> > function with the obvious rule everywhere that you call it holding lock,
> > no matter whether pc is or isn't NULL.  If somewhere in memcontrol.c
> > did call it without holding the lock, it'd also have to bizarrely
> > remember to unlock while forgetting to lock, for it to escape notice.
> 
> Yes, you are as always of-course right. I was thinking of future uses of the
> function. Some one could use it, a VM_BUG_ON will help.

In looking to reinstate that VM_BUG_ON, I notice that actually I'm
strictly wrong to be forcing the lock bit on there - because on UP
without DEBUG_SPINLOCK, the bit_spin_lock/unlock wouldn't use it,
so it'd be rather untidy to put it there and leave it in the assign.
I think the answer will be to #define PAGE_CGROUP_LOCK 0 for that case.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>