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@xxxxxxxxxx> > --- > 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 > >
