On Thu, 8 Sep 2011 08:00:36 -0700 (PDT)
Dan Magenheimer [off-list ref] wrote:

quoted

From: Andrew Morton [mailto:akpm@linux-foundation.org]
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:.

quoted

quoted

From: Dan Magenheimer <redacted>
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.

quoted

quoted

+/*
+ * 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@kvack.org.  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@kvack.org"> email@kvack.org </a>