On Thu, 25 Aug 2011 10:37:05 -0700 (PDT) Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote: > > From: KAMEZAWA Hiroyuki [mailto:kamezawa.hiroyu@xxxxxxxxxxxxxx] > > Subject: Re: Subject: [PATCH V7 2/4] mm: frontswap: core code > > BTW, Do I have a chance to implement frontswap accounting per cgroup > > (under memcg) ? Or Do I need to enable/disale switch for frontswap per memcg ? > > Do you think it is worth to do ? > > I'm not very familiar with cgroups or memcg but I think it may be possible > to implement transcendent memory with cgroup as the "guest" and the default > cgroup as the "host" to allow for more memory elasticity for cgroups. > (See http://lwn.net/Articles/454795/ for a good overview of all of > transcendent memory.) > Ok, I'll see it. I just wonder following case. Assume 2 memcgs. memcg X: memory limit = 300M. memcg Y: memory limit = 300M. This limitation is done for performance isolation. When using frontswap, X and Y can cause resource confliction in frontswap and performance of X and Y cannot be predictable. > > > +/* > > > + * This global enablement flag reduces overhead on systems where frontswap_ops > > > + * has not been registered, so is preferred to the slower alternative: a > > > + * function call that checks a non-global. > > > + */ > > > +int frontswap_enabled; > > > +EXPORT_SYMBOL(frontswap_enabled); > > > + > > > +/* useful stats available in /sys/kernel/mm/frontswap */ > > > +static unsigned long frontswap_gets; > > > +static unsigned long frontswap_succ_puts; > > > +static unsigned long frontswap_failed_puts; > > > +static unsigned long frontswap_flushes; > > > + > > > > What lock guard these ? swap_lock ? > > These are informational statistics so do not need to be protected > by a lock or an atomic-type. If an increment is lost due to a cpu > race, it is not a problem. > Hmm...Personally, I don't like incorrect counters. Could you add comments ? Or How anout using percpu_counter ? (see lib/percpu_counter.c) > > > +/* Called when a swap device is swapon'd */ > > > +void __frontswap_init(unsigned type) > > > +{ > > > + struct swap_info_struct *sis = swap_info[type]; > > > + > > > + BUG_ON(sis == NULL); > > > + if (sis->frontswap_map == NULL) > > > + return; > > > + if (frontswap_enabled) > > > + (*frontswap_ops.init)(type); > > > +} > > > +EXPORT_SYMBOL(__frontswap_init); > > > + > > > +/* > > > + * "Put" data from a page to frontswap and associate it with the page's > > > + * swaptype and offset. Page must be locked and in the swap cache. > > > + * If frontswap already contains a page with matching swaptype and > > > + * offset, the frontswap implmentation may either overwrite the data > > > + * and return success or flush the page from frontswap and return failure > > > + */ > > > > What lock should be held to guard global variables ? swap_lock ? > > Which global variables do you mean and in what routines? I think the > page lock is required for put/get (as documented in the comments) > but not the swap_lock. > My concern was race in counters. Even you allow race in frontswap_succ_puts++, Don't you need some lock for sis->frontswap_pages++ sis->frontswap_pages-- ? Thanks, -Kame -- 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/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>