Messages in this thread | | | Date | Thu, 7 Mar 2019 07:06:23 -0800 | From | Matthew Wilcox <> | Subject | Re: [PATCH v3] page cache: Store only head pages in i_pages |
| |
On Wed, Mar 06, 2019 at 02:36:35AM -0700, William Kucharski wrote: > > Other than the bug Song found in memfd_tag_pins(), I'd like to suggest two quick > but pedantic changes to mm/filemap.c: > > Though not modified in this patch, in line 284, the parenthesis should be moved > to after the period: > > * modified.) The function expects only THP head pages to be present in the
https://english.stackexchange.com/questions/6632/where-does-the-period-go-when-using-parentheses disagrees with you
> > + * Move to the next page in the vector if this is a small page > > + * or the index is of the last page in this compound page). > > A few lines later, there is an extraneous parenthesis, and the comment could be a bit > clearer. > > Might I suggest: > > * Move to the next page in the vector if this is a PAGESIZE > * page or if the index is of the last PAGESIZE page within > * this compound page. > > You can say "base" instead of "PAGESIZE," but "small" seems open to interpretation.
Agreed on the spurios close paren. The THP documentation prefers the term 'regular page', so I went with:
* Move to the next page in the vector if this is a regular * page or the index is of the last sub-page of this compound * page.
> I haven't run across any problems and have been hammering the code for over five days > without issue; all my testing was with transparent_hugepage/enabled set to > "always." > > Tested-by: William Kucharski <william.kucharski@oracle.com> > Reviewed-by: William Kucharski <william.kucharski@oracle.com>
Thanks!
| |