| Message ID | 20220128195642.416743-2-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/page_owner: Extend page_owner to show memcg | expand |
On Fri, Jan 28, 2022 at 02:56:41PM -0500, Waiman Long wrote: > In print_page_owner(), there is a repetitive check after each snprintf() > to make sure that the final length is less than the buffer size. Simplify > the code and make it easier to read by embedding the buffer length > check and increment inside the macro. This does not follow the kernel coding style of not putting control flow statements in macros.[1] > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > mm/page_owner.c | 50 +++++++++++++++++++++++-------------------------- > 1 file changed, 23 insertions(+), 27 deletions(-) And in the end you only saved 4 lines of code to read... Not worth it IMO. Ira [1] https://www.kernel.org/doc/html/v4.17/process/coding-style.html > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 99e360df9465..c52ce9d6bc3b 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -325,12 +325,20 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m, > seq_putc(m, '\n'); > } > > +#define SNPRINTF(_buf, _size, _len, _err, _fmt, ...) \ > + do { \ > + _len += snprintf(_buf + _len, _size - _len, _fmt, \ > + ##__VA_ARGS__); \ > + if (_len >= _size) \ > + goto _err; \ > + } while (0) > + > static ssize_t > print_page_owner(char __user *buf, size_t count, unsigned long pfn, > struct page *page, struct page_owner *page_owner, > depot_stack_handle_t handle) > { > - int ret, pageblock_mt, page_mt; > + int ret = 0, pageblock_mt, page_mt; > char *kbuf; > > count = min_t(size_t, count, PAGE_SIZE); > @@ -338,44 +346,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > if (!kbuf) > return -ENOMEM; > > - ret = snprintf(kbuf, count, > - "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", > - page_owner->order, page_owner->gfp_mask, > - &page_owner->gfp_mask, page_owner->pid, > - page_owner->ts_nsec, page_owner->free_ts_nsec); > - > - if (ret >= count) > - goto err; > + SNPRINTF(kbuf, count, ret, err, > + "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", > + page_owner->order, page_owner->gfp_mask, > + &page_owner->gfp_mask, page_owner->pid, > + page_owner->ts_nsec, page_owner->free_ts_nsec); > > /* Print information relevant to grouping pages by mobility */ > pageblock_mt = get_pageblock_migratetype(page); > page_mt = gfp_migratetype(page_owner->gfp_mask); > - ret += snprintf(kbuf + ret, count - ret, > - "PFN %lu type %s Block %lu type %s Flags %pGp\n", > - pfn, > - migratetype_names[page_mt], > - pfn >> pageblock_order, > - migratetype_names[pageblock_mt], > - &page->flags); > - > - if (ret >= count) > - goto err; > + SNPRINTF(kbuf, count, ret, err, > + "PFN %lu type %s Block %lu type %s Flags %pGp\n", > + pfn, migratetype_names[page_mt], > + pfn >> pageblock_order, > + migratetype_names[pageblock_mt], > + &page->flags); > > ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0); > if (ret >= count) > goto err; > > - if (page_owner->last_migrate_reason != -1) { > - ret += snprintf(kbuf + ret, count - ret, > + if (page_owner->last_migrate_reason != -1) > + SNPRINTF(kbuf, count, ret, err, > "Page has been migrated, last migrate reason: %s\n", > migrate_reason_names[page_owner->last_migrate_reason]); > - if (ret >= count) > - goto err; > - } > > - ret += snprintf(kbuf + ret, count - ret, "\n"); > - if (ret >= count) > - goto err; > + SNPRINTF(kbuf, count, ret, err, "\n"); > > if (copy_to_user(buf, kbuf, ret)) > ret = -EFAULT; > -- > 2.27.0 > >
On 1/28/22 16:18, Ira Weiny wrote: > On Fri, Jan 28, 2022 at 02:56:41PM -0500, Waiman Long wrote: >> In print_page_owner(), there is a repetitive check after each snprintf() >> to make sure that the final length is less than the buffer size. Simplify >> the code and make it easier to read by embedding the buffer length >> check and increment inside the macro. > This does not follow the kernel coding style of not putting control flow > statements in macros.[1] OK, fair enough. I can remove the macro and repost the patch. Cheers, Longman >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> mm/page_owner.c | 50 +++++++++++++++++++++++-------------------------- >> 1 file changed, 23 insertions(+), 27 deletions(-) > And in the end you only saved 4 lines of code to read... Not worth it IMO. > > Ira > > [1] https://www.kernel.org/doc/html/v4.17/process/coding-style.html > >> diff --git a/mm/page_owner.c b/mm/page_owner.c >> index 99e360df9465..c52ce9d6bc3b 100644 >> --- a/mm/page_owner.c >> +++ b/mm/page_owner.c >> @@ -325,12 +325,20 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m, >> seq_putc(m, '\n'); >> } >> >> +#define SNPRINTF(_buf, _size, _len, _err, _fmt, ...) \ >> + do { \ >> + _len += snprintf(_buf + _len, _size - _len, _fmt, \ >> + ##__VA_ARGS__); \ >> + if (_len >= _size) \ >> + goto _err; \ >> + } while (0) >> + >> static ssize_t >> print_page_owner(char __user *buf, size_t count, unsigned long pfn, >> struct page *page, struct page_owner *page_owner, >> depot_stack_handle_t handle) >> { >> - int ret, pageblock_mt, page_mt; >> + int ret = 0, pageblock_mt, page_mt; >> char *kbuf; >> >> count = min_t(size_t, count, PAGE_SIZE); >> @@ -338,44 +346,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, >> if (!kbuf) >> return -ENOMEM; >> >> - ret = snprintf(kbuf, count, >> - "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", >> - page_owner->order, page_owner->gfp_mask, >> - &page_owner->gfp_mask, page_owner->pid, >> - page_owner->ts_nsec, page_owner->free_ts_nsec); >> - >> - if (ret >= count) >> - goto err; >> + SNPRINTF(kbuf, count, ret, err, >> + "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", >> + page_owner->order, page_owner->gfp_mask, >> + &page_owner->gfp_mask, page_owner->pid, >> + page_owner->ts_nsec, page_owner->free_ts_nsec); >> >> /* Print information relevant to grouping pages by mobility */ >> pageblock_mt = get_pageblock_migratetype(page); >> page_mt = gfp_migratetype(page_owner->gfp_mask); >> - ret += snprintf(kbuf + ret, count - ret, >> - "PFN %lu type %s Block %lu type %s Flags %pGp\n", >> - pfn, >> - migratetype_names[page_mt], >> - pfn >> pageblock_order, >> - migratetype_names[pageblock_mt], >> - &page->flags); >> - >> - if (ret >= count) >> - goto err; >> + SNPRINTF(kbuf, count, ret, err, >> + "PFN %lu type %s Block %lu type %s Flags %pGp\n", >> + pfn, migratetype_names[page_mt], >> + pfn >> pageblock_order, >> + migratetype_names[pageblock_mt], >> + &page->flags); >> >> ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0); >> if (ret >= count) >> goto err; >> >> - if (page_owner->last_migrate_reason != -1) { >> - ret += snprintf(kbuf + ret, count - ret, >> + if (page_owner->last_migrate_reason != -1) >> + SNPRINTF(kbuf, count, ret, err, >> "Page has been migrated, last migrate reason: %s\n", >> migrate_reason_names[page_owner->last_migrate_reason]); >> - if (ret >= count) >> - goto err; >> - } >> >> - ret += snprintf(kbuf + ret, count - ret, "\n"); >> - if (ret >= count) >> - goto err; >> + SNPRINTF(kbuf, count, ret, err, "\n"); >> >> if (copy_to_user(buf, kbuf, ret)) >> ret = -EFAULT; >> -- >> 2.27.0 >> >>
diff --git a/mm/page_owner.c b/mm/page_owner.c index 99e360df9465..c52ce9d6bc3b 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -325,12 +325,20 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m, seq_putc(m, '\n'); } +#define SNPRINTF(_buf, _size, _len, _err, _fmt, ...) \ + do { \ + _len += snprintf(_buf + _len, _size - _len, _fmt, \ + ##__VA_ARGS__); \ + if (_len >= _size) \ + goto _err; \ + } while (0) + static ssize_t print_page_owner(char __user *buf, size_t count, unsigned long pfn, struct page *page, struct page_owner *page_owner, depot_stack_handle_t handle) { - int ret, pageblock_mt, page_mt; + int ret = 0, pageblock_mt, page_mt; char *kbuf; count = min_t(size_t, count, PAGE_SIZE); @@ -338,44 +346,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, if (!kbuf) return -ENOMEM; - ret = snprintf(kbuf, count, - "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", - page_owner->order, page_owner->gfp_mask, - &page_owner->gfp_mask, page_owner->pid, - page_owner->ts_nsec, page_owner->free_ts_nsec); - - if (ret >= count) - goto err; + SNPRINTF(kbuf, count, ret, err, + "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", + page_owner->order, page_owner->gfp_mask, + &page_owner->gfp_mask, page_owner->pid, + page_owner->ts_nsec, page_owner->free_ts_nsec); /* Print information relevant to grouping pages by mobility */ pageblock_mt = get_pageblock_migratetype(page); page_mt = gfp_migratetype(page_owner->gfp_mask); - ret += snprintf(kbuf + ret, count - ret, - "PFN %lu type %s Block %lu type %s Flags %pGp\n", - pfn, - migratetype_names[page_mt], - pfn >> pageblock_order, - migratetype_names[pageblock_mt], - &page->flags); - - if (ret >= count) - goto err; + SNPRINTF(kbuf, count, ret, err, + "PFN %lu type %s Block %lu type %s Flags %pGp\n", + pfn, migratetype_names[page_mt], + pfn >> pageblock_order, + migratetype_names[pageblock_mt], + &page->flags); ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0); if (ret >= count) goto err; - if (page_owner->last_migrate_reason != -1) { - ret += snprintf(kbuf + ret, count - ret, + if (page_owner->last_migrate_reason != -1) + SNPRINTF(kbuf, count, ret, err, "Page has been migrated, last migrate reason: %s\n", migrate_reason_names[page_owner->last_migrate_reason]); - if (ret >= count) - goto err; - } - ret += snprintf(kbuf + ret, count - ret, "\n"); - if (ret >= count) - goto err; + SNPRINTF(kbuf, count, ret, err, "\n"); if (copy_to_user(buf, kbuf, ret)) ret = -EFAULT;
In print_page_owner(), there is a repetitive check after each snprintf() to make sure that the final length is less than the buffer size. Simplify the code and make it easier to read by embedding the buffer length check and increment inside the macro. Signed-off-by: Waiman Long <longman@redhat.com> --- mm/page_owner.c | 50 +++++++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 27 deletions(-)