On Fri 18-08-17 20:41:45, Andrea Arcangeli wrote: > On Fri, Aug 18, 2017 at 09:04:44AM +0200, Michal Hocko wrote: > > I dunno. This doesn't make any difference in the generated code for > > me (with gcc 6.4). If anything we might wan't to putt unlikely inside > > That's fine, this is just in case the code surrounding the check > changes in the future. It's not like we should remove unlikely/likely > if the emitted bytecode doesn't change. > > > tsk_is_oom_victim. Or even go further and use a jump label to get any > > I don't think it's necessarily the best to put it inside > tsk_is_oom_victim, even if currently it would be the same. > > All it matters for likely unlikely is not to risk to ever get it > wrong. If unsure it's better to leave it alone. > > We can't be sure all future callers of tsk_is_oom_victim will always > be unlikely to get a true retval. All we can be sure is that this > specific caller will get a false retval 100% of the time, in all > workloads where performance can matter. Cosindering that it is highly unlikely to meet an OOM victim I would consider unlikely as always applicable. Even if this is something in the oom proper then it is a) a cold path so a misprediction doesn't matter and b) even then it is highly unlikely to meet a victim because oom victims should almost always be a minority. > > conditional paths out of way. > > Using a jump label won't allocate memory so I tend to believe it would > be safe to run them here. However before worrying at the exit path, I > think the first target of optimization would be the MMF_UNSTABLE > checks, those are in the page fault fast paths and they end up run > infinitely more frequently than this single branch in exit. Yes that is true. [...] > So what would you think about the simplest approach to the > MMF_UNSTABLE issue, that is to add a build time CONFIG_OOM_REAPER=y > option for the OOM reaper so those branches are optimized away at > build time (and the above one too, and perhaps the MMF_OOM_SKIP > set_bit too) if it's ok to disable the OOM reaper as well and increase > the risk an OOM hang? (it's years I didn't hit an OOM hang in my > desktop even before OOM reaper was introduced). It could be default > enabled of course. I really do hate how many config options we have already and adding more on top doesn't look like an improvement to me. Jump labels sound like a much better way forward. Or do you see any potential disadvantage? > I'd be curious to be able to still test what happens to the VM when > the OOM reaper is off, so if nothing else it would be a debug option, > because it'd also help to reproduce more easily those The same could be achieved with a kernel command line option which would be a smaller patch, easier to maintain in future and also wouldn't further increase the config space fragmentation. > filesystem-kernel-thread induced hangs that would still happen if the > OOM reaper cannot run because some other process is trying to take the > mmap_sem for writing. A down_read_trylock_unfair would go a long way > to reduce the likelyhood to run into that. The kernel CI exercising > multiple configs would then also autonomously CC us on a report if > those branches are a measurable issue so it'll be easier to tell if > the migration entry conversion or static key is worth it for > MMF_UNSTABLE. While this sounds like an interesting exercise I am not convinced it justifies the new config option. -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
