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 <[email protected]> 1.925.600.0401
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
| |
 |