Messages in this thread | | | Date | Tue, 1 Sep 2009 16:55:49 +0800 | From | Wu Fengguang <> | Subject | Re: [RFC][PATCH 0/4] memcg: add support for hwpoison testing |
| |
On Tue, Sep 01, 2009 at 03:12:28PM +0800, KAMEZAWA Hiroyuki wrote: > On Tue, 1 Sep 2009 14:46:52 +0800 > Wu Fengguang <fengguang.wu@intel.com> wrote: > > > On Tue, Sep 01, 2009 at 10:32:14AM +0800, KAMEZAWA Hiroyuki wrote: > > > On Tue, 1 Sep 2009 10:25:14 +0800 > > > Wu Fengguang <fengguang.wu@intel.com> wrote: > > > > > 4. I can't understand why you need this. I wonder you can get pfn via > > > > > /proc/<pid>/????. And this may insert HWPOISON to page-cache of shared > > > > > library and "unexpected" process will be poisoned. > > > > > > > > Sorry I should have explained this. It's mainly for correctness. > > > > When a user space tool queries the task PFNs in /proc/pid/pagemap and > > > > then send to /debug/hwpoison/corrupt-pfn, there is a racy window that > > > > the page could be reclaimed and allocated by some one else. It would > > > > be awkward to try to pin the pages in user space. So we need the > > > > guarantees provided by /debug/hwpoison/corrupt-filter-memcg, which > > > > will be checked inside the page lock with elevated reference count. > > > > > > > > > > memcg never holds refcnt for a page and the kernel::vmscan.c can reclaim > > > any pages under memcg whithout checking anything related to memcg. > > > *And*, your code has no "pin" code. > > > This patch sed does no jobs for your concern. > > > > We grabbed page here, which is not in the scope of this patchset: > > > > static int try_memory_failure(unsigned long pfn) > > { > > struct page *p; > > int res = -EINVAL; > > > > if (!pfn_valid(pfn)) > > return res; > > > > p = pfn_to_page(pfn); > > if (!get_page_unless_zero(compound_head(p))) > > return res; > > > > lock_page_nosync(compound_head(p)); > > > > if (hwpoison_filter(p)) > > goto out; > > > > res = __memory_failure(pfn, 18, > > MEMORY_FAILURE_FLAG_COUNTED | > > MEMORY_FAILURE_FLAG_LOCKED); > > out: > > unlock_page(p); > > return res; > > } > > Hmm. maybe off-topic but why lock_page() is necessary ?
Because we also have filter for testing page flags, which requires lock_page() to be correct.
> > > > I recommend you to add > > > /debug/hwpoizon/pin-pfn > > > > > > Then, > > > echo pfn > /debug/hwpoizon/pin-pfn > > > # add pfn for hwpoison debug's watch list. and elevate refcnt > > > check 'pfn' is still used. > > > echo pfn > /debug/hwpoison/corrupt-pfn > > > # check 'watch list' and make it corrupt and release refcnt. > > > or some. > > > > Looks like a good alternative. At least no more memcg dependency.. > > > > My point is that memcg can show 'owner' of pages but the page may > be shared with something important task _and_ if a task is migrated, > its pages' memcg information is not updated now. Then, you can kill > a task which is not in memcg.
Ah thanks! I'm not aware of that tricky fact, and it does make a very good reason not to use memcg, although I guess locked page won't be migrated.
> Then, I don't recommend to use memcg. I think you'll see too much > pitfalls.
Thanks, Fengguang
| |