On Thu, 8 Sep 2011 08:00:36 -0700 (PDT) Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> wrote: > > From: Andrew Morton [mailto:akpm@xxxxxxxxxxxxxxxxxxxx] > > Subject: Re: [PATCH V8 2/4] mm: frontswap: core code > > Thanks very much for taking the time for this feedback! > > Please correct me if I am presumptuous or misreading > SubmittingPatches, but after making the changes below, > I am thinking this constitutes a "Reviewed-by"? Not really. More like Briefly-browsed-by:. > > > From: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx> > > > Subject: [PATCH V8 2/4] mm: frontswap: core code > > > > > > This second patch of four in this frontswap series provides the core code > > > for frontswap that interfaces between the hooks in the swap subsystem and > > > + > > > +struct frontswap_ops { > > > + void (*init)(unsigned); > > > + int (*put_page)(unsigned, pgoff_t, struct page *); > > > + int (*get_page)(unsigned, pgoff_t, struct page *); > > > + void (*flush_page)(unsigned, pgoff_t); > > > + void (*flush_area)(unsigned); > > > +}; > > > > Please don't use the term "flush". In both the pagecache code and the > > pte code it is interchangably used to refer to both writeback and > > invalidation. The way to avoid this ambiguity and confusion is to use > > the terms "writeback" and "invalidate" instead. > > > > Here, you're referring to invalidation. > > While the different name is OK, changing this consistently would now > require simultaneous patches in cleancache, zcache, and xen (not > to mention lots of docs inside and outside the kernel). I suspect > it would be cleaner to do this later across all affected code > with a single commit. Hope that's OK. Well, if you can make that happen... > (Personally, I find "invalidate" to be inaccurate because common > usage of the term doesn't imply that the space used in the cache > is recovered, i.e. garbage collection, which is the case here. > To me, "flush" implies invalidate PLUS recover space.) invalidate is close enough. Consider block/blk-flush.c, sigh. > > > > +/* > > > + * Useful stats available in /sys/kernel/mm/frontswap. These are for > > > + * information only so are not protected against increment/decrement races. > > > + */ > > > +static unsigned long frontswap_gets; > > > +static unsigned long frontswap_succ_puts; > > > +static unsigned long frontswap_failed_puts; > > > +static unsigned long frontswap_flushes; > > > > If they're in /sys/kernel/mm then they rather become permanent parts of > > the exported kernel interface. We're stuck with them. Plus they're > > inaccurate and updating them might be inefficient, so we don't want to > > be stuck with them. > > > > I suggest moving these to debugfs from where we can remove them if we > > feel like doing so. > > The style (and code) for this was mimicked from ksm and hugepages, which > expose the stats the same way... as does cleancache now. slub is also > similar. I'm OK with using a different approach (e.g. debugfs), but > think it would be inconsistent and confusing to expose these stats > differently than cleancache (or ksm and hugepages). I'd support > and help with a massive cleanup commit across all of mm later though. > Hope that's OK for now. These are boring internal counters for a few developers. They're so uninteresting to end users that the developer didn't even bother to document them ;) They should be in debugfs. Probably some/all of the existing cleancache/ksm/hugepage stats should be in debugfs too. This a mistake we often make. Please let's be extremely miserly with the kernel API. -- 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>