From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: Greg Thelen <gthelen@google.com>, Johannes Weiner <hannes@cmpxchg.org>, David Rientjes <rientjes@google.com>, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>, Minchan Kim <minchan.kim@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory() Date: Thu, 27 Jan 2011 09:24:34 +0900 [thread overview] Message-ID: <20110127092434.df18c7a6.kamezawa.hiroyu@jp.fujitsu.com> (raw) In-Reply-To: <20110126142909.0b710a0c.akpm@linux-foundation.org> On Wed, 26 Jan 2011 14:29:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 26 Jan 2011 12:32:04 -0800 > Greg Thelen <gthelen@google.com> wrote: > > > > That being said, does this have any practical impact at all? I mean, > > > this code runs when the cgroup limit is breached. But if the number > > > of allowed pages (not bytes!) can not fit into 32 bits, it means you > > > have a group of processes using more than 16T. On a 32-bit machine. > > > > The value of this patch is up for debate. I do not have an example > > situation where this truncation causes the wrong thing to happen. I > > suppose it might be possible for a racing update to > > memory.limit_in_bytes which grows the limit from a reasonable (example: > > 100M) limit to a large limit (example 1<<45) could benefit from this > > patch. I admit that this case seems pathological and may not be likely > > or even worth bothering over. If neither the memcg nor the oom > > maintainers want the patch, then feel free to drop it. I just noticed > > the issue and thought it might be worth addressing. > > Ah. I was scratching my head over that. > > In zillions of places the kernel assumes that a 32-bit kernel has less > than 2^32 pages of memory, so the code as it stands is, umm, idiomatic. > I think we can assume that. > But afaict the only way the patch makes a real-world difference is if > res_counter_read_u64() is busted? > > And, as you point out, res_counter_read_u64() is indeed busted on > 32-bit machines. It has 25 callsites in mm/memcontrol.c - has anyone > looked at the implications of this? What happens in all those > callsites if the counter is read during a count rollover? > I'll review. Against the roll-over, I think we just need to take lock. So, res_counter_read_u64() implementation was wrong. It should take lock. Please give me time. THanks, -Kame
WARNING: multiple messages have this Message-ID (diff)
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: Greg Thelen <gthelen@google.com>, Johannes Weiner <hannes@cmpxchg.org>, David Rientjes <rientjes@google.com>, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>, Minchan Kim <minchan.kim@gmail.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] oom: handle overflow in mem_cgroup_out_of_memory() Date: Thu, 27 Jan 2011 09:24:34 +0900 [thread overview] Message-ID: <20110127092434.df18c7a6.kamezawa.hiroyu@jp.fujitsu.com> (raw) In-Reply-To: <20110126142909.0b710a0c.akpm@linux-foundation.org> On Wed, 26 Jan 2011 14:29:09 -0800 Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 26 Jan 2011 12:32:04 -0800 > Greg Thelen <gthelen@google.com> wrote: > > > > That being said, does this have any practical impact at all? I mean, > > > this code runs when the cgroup limit is breached. But if the number > > > of allowed pages (not bytes!) can not fit into 32 bits, it means you > > > have a group of processes using more than 16T. On a 32-bit machine. > > > > The value of this patch is up for debate. I do not have an example > > situation where this truncation causes the wrong thing to happen. I > > suppose it might be possible for a racing update to > > memory.limit_in_bytes which grows the limit from a reasonable (example: > > 100M) limit to a large limit (example 1<<45) could benefit from this > > patch. I admit that this case seems pathological and may not be likely > > or even worth bothering over. If neither the memcg nor the oom > > maintainers want the patch, then feel free to drop it. I just noticed > > the issue and thought it might be worth addressing. > > Ah. I was scratching my head over that. > > In zillions of places the kernel assumes that a 32-bit kernel has less > than 2^32 pages of memory, so the code as it stands is, umm, idiomatic. > I think we can assume that. > But afaict the only way the patch makes a real-world difference is if > res_counter_read_u64() is busted? > > And, as you point out, res_counter_read_u64() is indeed busted on > 32-bit machines. It has 25 callsites in mm/memcontrol.c - has anyone > looked at the implications of this? What happens in all those > callsites if the counter is read during a count rollover? > I'll review. Against the roll-over, I think we just need to take lock. So, res_counter_read_u64() implementation was wrong. It should take lock. Please give me time. THanks, -Kame -- 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/ . Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2011-01-27 0:30 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-01-26 8:29 [PATCH] oom: handle overflow in mem_cgroup_out_of_memory() Greg Thelen 2011-01-26 8:29 ` Greg Thelen 2011-01-26 13:40 ` Balbir Singh 2011-01-26 13:40 ` Balbir Singh 2011-01-26 17:07 ` Johannes Weiner 2011-01-26 17:07 ` Johannes Weiner 2011-01-26 17:33 ` Greg Thelen 2011-01-26 17:33 ` Greg Thelen 2011-01-26 18:30 ` Johannes Weiner 2011-01-26 18:30 ` Johannes Weiner 2011-01-26 20:32 ` Greg Thelen 2011-01-26 20:32 ` Greg Thelen 2011-01-26 22:29 ` Andrew Morton 2011-01-26 22:29 ` Andrew Morton 2011-01-27 0:24 ` KAMEZAWA Hiroyuki [this message] 2011-01-27 0:24 ` KAMEZAWA Hiroyuki 2011-01-27 0:53 ` [BUGFIX] memcg: fix res_counter_read_u64 lock aware (Was " KAMEZAWA Hiroyuki 2011-01-27 0:53 ` KAMEZAWA Hiroyuki 2011-01-27 1:08 ` Andrew Morton 2011-01-27 1:08 ` Andrew Morton 2011-01-27 1:24 ` KAMEZAWA Hiroyuki 2011-01-27 1:24 ` KAMEZAWA Hiroyuki 2011-01-27 1:34 ` KAMEZAWA Hiroyuki 2011-01-27 1:34 ` KAMEZAWA Hiroyuki 2011-01-27 1:55 ` Andrew Morton 2011-01-27 1:55 ` Andrew Morton 2011-01-27 1:43 ` [BUGFIX v2] " KAMEZAWA Hiroyuki 2011-01-27 1:43 ` KAMEZAWA Hiroyuki 2011-01-27 1:57 ` Andrew Morton 2011-01-27 1:57 ` Andrew Morton 2011-01-27 2:13 ` KAMEZAWA Hiroyuki 2011-01-27 2:13 ` KAMEZAWA Hiroyuki 2011-01-27 0:40 ` KAMEZAWA Hiroyuki 2011-01-27 0:40 ` KAMEZAWA Hiroyuki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110127092434.df18c7a6.kamezawa.hiroyu@jp.fujitsu.com \ --to=kamezawa.hiroyu@jp.fujitsu.com \ --cc=akpm@linux-foundation.org \ --cc=gthelen@google.com \ --cc=hannes@cmpxchg.org \ --cc=kosaki.motohiro@jp.fujitsu.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=minchan.kim@gmail.com \ --cc=rientjes@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.