Skip to contentSkip to conversation

mm: add comments on pglist_data zones

PATCH Applied Ben Widawsky 2 participants 4 messages May 20, 2020 – May 21, 2020 Memory management

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mm: add comments on pglist_data zones

While making other modifications it was easy to confuse the two struct members node_zones and node_zonelists. For those already familiar with the code, this might seem to be a silly patch, but it's quite helpful to disambiguate the similar-sounding fields

While here, add a small comment on why nr_zones isn't simply MAX_NR_ZONES

---
 include/linux/mmzone.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
linux/mmzone.h +13 -1
index 1b9de7d220fb..4f7c95e9bad8 100644
@@ -665,9 +665,21 @@ struct deferred_split {
  * per-zone basis.
  */
 typedef struct pglist_data {
+	/*
+	 * node_zones contains just the zones for THIS node. Not all of the
+	 * zones may be populated, but it is the full list. It is referenced by
+	 * this node's node_zonelists as well as other node's node_zonelists.
+	 */
 	struct zone node_zones[MAX_NR_ZONES];
+
+	/*
+	 * node_zonelists contains references to all zones in all nodes.
+	 * Generally the first zones will be references to this node's
+	 * node_zones.
+	 */
 	struct zonelist node_zonelists[MAX_ZONELISTS];
-	int nr_zones;
+
+	int nr_zones; /* number of populated zones in this node */
 #ifdef CONFIG_FLAT_NODE_MEM_MAP	/* means !SPARSEMEM */
 	struct page *node_mem_map;
 #ifdef CONFIG_PAGE_EXTENSION
Signature · 1 line
2.26.2
#1 lore raw
Quoted 6 lines · Ben Widawsky

While making other modifications it was easy to confuse the two struct members node_zones and node_zonelists. For those already familiar with the code, this might seem to be a silly patch, but it's quite helpful to disambiguate the similar-sounding fields

While here, add a small comment on why nr_zones isn't simply MAX_NR_ZONES

It seems like a real shame to write all this excellent documentation and not format it as kernel-doc.

Quoted 36 lines
---
 include/linux/mmzone.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
linux/mmzone.h +13 -1
index 1b9de7d220fb..4f7c95e9bad8 100644
@@ -665,9 +665,21 @@ struct deferred_split {
  * per-zone basis.
  */
 typedef struct pglist_data {
+	/*
+	 * node_zones contains just the zones for THIS node. Not all of the
+	 * zones may be populated, but it is the full list. It is referenced by
+	 * this node's node_zonelists as well as other node's node_zonelists.
+	 */
 	struct zone node_zones[MAX_NR_ZONES];
+
+	/*
+	 * node_zonelists contains references to all zones in all nodes.
+	 * Generally the first zones will be references to this node's
+	 * node_zones.
+	 */
 	struct zonelist node_zonelists[MAX_ZONELISTS];
-	int nr_zones;
+
+	int nr_zones; /* number of populated zones in this node */
 #ifdef CONFIG_FLAT_NODE_MEM_MAP	/* means !SPARSEMEM */
 	struct page *node_mem_map;
 #ifdef CONFIG_PAGE_EXTENSION
Signature · 1 line
2.26.2
#2 lore raw
Quoted 10 lines · Matthew Wilcox
Quoted 6 lines · Ben Widawsky

While making other modifications it was easy to confuse the two struct members node_zones and node_zonelists. For those already familiar with the code, this might seem to be a silly patch, but it's quite helpful to disambiguate the similar-sounding fields

While here, add a small comment on why nr_zones isn't simply MAX_NR_ZONES

It seems like a real shame to write all this excellent documentation and not format it as kernel-doc.

I admit, I didn't look at all the kernel-doc files. Is there precedent there for documenting struct members like this? I'd be more than happy to try to document everything I've dug up in coming up to speed here.

I've used the docs from Mel quite a bit and would very much like to pay it forward, as it were.

#3 lore raw
Quoted 12 lines · Ben Widawsky
Quoted 10 lines · Matthew Wilcox
Quoted 6 lines · Ben Widawsky

While making other modifications it was easy to confuse the two struct members node_zones and node_zonelists. For those already familiar with the code, this might seem to be a silly patch, but it's quite helpful to disambiguate the similar-sounding fields

While here, add a small comment on why nr_zones isn't simply MAX_NR_ZONES

It seems like a real shame to write all this excellent documentation and not format it as kernel-doc.

Quoted 6 lines

I admit, I didn't look at all the kernel-doc files. Is there precedent there for documenting struct members like this? I'd be more than happy to try to document everything I've dug up in coming up to speed here.

I've used the docs from Mel quite a bit and would very much like to pay it forward, as it were.

I appreciate your willingness! Fortunately, we have excellent documentation on adding more documentation ;-)

Documentation/doc-guide/kernel-doc.rst is where you'll want to start. 'Structure, union, and enumeration documentation' is the section. Do read through the whole section before jumping in because you might find it more clear to use the style in 'In-line member documentation comments' rather than the style documented first.