Messages in this thread | | | Date | Mon, 26 Sep 2005 22:44:39 -0700 | From | Paul Jackson <> | Subject | Re: [PATCH 1/9] add defrag flags |
| |
Dave wrote: > I think Joel simply made an error in his description.
Looks like he made the same mistake in the actual code comments:
+/* Allocation type modifiers, group together if possible + * __GPF_USER: Allocation for user page or a buffer page + * __GFP_KERNRCLM: Short-lived or reclaimable kernel allocation + */ +#define __GFP_USER 0x40000u /* Kernel page that is easily reclaimable */ +#define __GFP_KERNRCLM 0x80000u /* User is a userspace user */
I'd guess you meant to write more like the following:
#define __GFP_USER 0x40000u /* Page for user address space */ #define __GFP_KERNRCLM 0x80000u /* Kernel page that is easily reclaimable */
And the block comment seems to needlessly repeat the inline comments, add a dubious claim, and omit the interesting stuff ... In other words:
Does it actually matter if these two bits are grouped, or not? I suspect that some of your other code, such as shifting the gfpmask by RCLM_SHIFT bits, _requires_ that these two bits be adjacent. So the "if possible" in the comment above is misleading.
And I suspect that gfp.h should contain the RCLM_SHIFT define, or at least mention in comment that RCLM_SHIFT depends on the position of the above two __GFP_* bits.
And I don't see any mention in the comments in gfp.h that these two bits, in tandem, have an additional meaning - both bits off means, I guess, not reclaimable, well at least not easily.
My HARDWALL patch appears to already be in Linus's kernel, so you probably also need to do a global substitute of all instances in the kernel of __GFP_HARDWALL, replacing it with __GFP_USER. Here is the list of files I see affected, with a count of the number of __GFP_HARDWALL strings in each:
include/linux/gfp.h:4 kernel/cpuset.c:6 mm/page_alloc.c:2 mm/vmscan.c:4
The comment in the next line looks like it needs to be changed to match the code change:
+#define __GFP_BITS_SHIFT 21 /* Room for 20 __GFP_FOO bits */
On the other hand, why did you change __GFP_BITS_SHIFT? Isn't 20 enough - just enough?
Why was the flag change in fs/buffer.c:grow_dev_page() to add the __GFP_USER bit, not to add the __GFP_KERNRCLM bit? I don't know that code - perhaps the answer is simply that the resulting page ends up in user space.
Aha - I just read one of the comments above that I cut+pasted. It says that __GFP_USER means user *OR* buffer page. That certainly explains the fs/buffer.c code using __GFP_USER. But it causes me to wonder if we can equate __GFP_USER with __GFP_HARDWALL. I'm reluctant, but more on principal than concrete experience, to modify the meaning of hardwall cpusets to constrain both user address space pages *AND* buffer pages. How open would you be to making buffers __GFP_KERNRCLM instead of __GFP_USER?
If you have good reason to keep __GFP_USER meanin either user or buffer, then perhaps the name __GFP_USER is misleading.
What sort of performance claims can you make for this change? How does it impact kernel text size? Could we see a diffstat for the entire patchset? Under what sort of loads or conditions would you expect this patchset to do more harm than good?
-- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |