On Thu 12-01-17 14:12:47, Minchan Kim wrote: > Hello, > > On Wed, Jan 11, 2017 at 04:52:39PM +0100, Michal Hocko wrote: > > On Wed 11-01-17 08:52:50, Minchan Kim wrote: > > [...] > > > > @@ -2055,8 +2055,8 @@ static bool inactive_list_is_low(struct > > > > if (!file && !total_swap_pages) > > > > return false; > > > > > > > > - inactive = lruvec_lru_size(lruvec, file * LRU_FILE); > > > > - active = lruvec_lru_size(lruvec, file * LRU_FILE + LRU_ACTIVE); > > > > + total_inactive = inactive = lruvec_lru_size(lruvec, file * LRU_FILE); > > > > + total_active = active = lruvec_lru_size(lruvec, file * LRU_FILE + LRU_ACTIVE); > > > > > > > > > > the decision of deactivating is based on eligible zone's LRU size, > > > not whole zone so why should we need to get a trace of all zones's LRU? > > > > Strictly speaking, the total_ counters are not necessary for making the > > decision. I found reporting those numbers useful regardless because this > > will give us also an information how large is the eligible portion of > > the LRU list. We do not have any other tracepoint which would report > > that. > > The patch doesn't say anything why it's useful. Could you tell why it's > useful and inactive_list_is_low should be right place? > > Don't get me wrong, please. I don't want to bother you. > I really don't want to add random stuff although it's tracepoint for > debugging. This doesn't sounds random to me. We simply do not have a full picture on 32b systems without this information. Especially when memcgs are involved and global numbers spread over different LRUs. > > [...] > > > > @@ -2223,7 +2228,7 @@ static void get_scan_count(struct lruvec > > > > * lruvec even if it has plenty of old anonymous pages unless the > > > > * system is under heavy pressure. > > > > */ > > > > - if (!inactive_list_is_low(lruvec, true, sc) && > > > > + if (!inactive_list_is_low(lruvec, true, sc, false) && > > > > > > Hmm, I was curious why you added trace boolean arguement and found it here. > > > Yes, here is not related to deactivation directly but couldn't we help to > > > trace it unconditionally? > > > > I've had it like that when I was debugging the mentioned bug and found > > it a bit disturbing. It generated more output than I would like and it > > wasn't really clear from which code path this has been called from. > > Indeed. > > Personally, I want to move inactive_list_is_low in shrink_active_list > and shrink_active_list calls inactive_list_is_low(...., true), > unconditionally so that it can make code simple/clear but cannot remove > trace boolean variable , which what I want. So, it's okay if you love > your version. I am not sure I am following. Why is the additional parameter a problem? -- 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>