lkml.org 
[lkml]   [2023]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs
    On Thu, Feb 02, 2023 at 04:29:12PM +0500, Muhammad Usama Anjum wrote:
    ...
    Hi Muhammad! I'm really sorry for not commenting this code, just out of time and i
    fear cant look with precise care at least for some time, hopefully other CRIU guys
    pick it up. Anyway, here a few comment from a glance.

    > +
    > +static inline int pagemap_scan_output(bool wt, bool file, bool pres, bool swap,
    > + struct pagemap_scan_private *p, unsigned long addr,
    > + unsigned int len)
    > +{

    This is a big function and usually it's a flag to not declare it as "inline" until
    there very serious reson to.

    > + unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap);
    > + bool cpy = true;
    > + struct page_region *prev = &p->prev;
    > +
    > + if (HAS_NO_SPACE(p))
    > + return -ENOSPC;
    > +
    > + if (p->max_pages && p->found_pages + len >= p->max_pages)
    > + len = p->max_pages - p->found_pages;
    > + if (!len)
    > + return -EINVAL;
    > +
    > + if (p->required_mask)
    > + cpy = ((p->required_mask & cur) == p->required_mask);
    > + if (cpy && p->anyof_mask)
    > + cpy = (p->anyof_mask & cur);
    > + if (cpy && p->excluded_mask)
    > + cpy = !(p->excluded_mask & cur);
    > + bitmap = cur & p->return_mask;
    > + if (cpy && bitmap) {

    You can exit early here simply

    if (!cpy || !bitmap)
    return 0;

    saving one tab for the code below.

    > + if ((prev->len) && (prev->bitmap == bitmap) &&
    > + (prev->start + prev->len * PAGE_SIZE == addr)) {
    > + prev->len += len;
    > + p->found_pages += len;
    > + } else if (p->vec_index < p->vec_len) {
    > + if (prev->len) {
    > + memcpy(&p->vec[p->vec_index], prev, sizeof(struct page_region));
    > + p->vec_index++;
    > + }
    > + prev->start = addr;
    > + prev->len = len;
    > + prev->bitmap = bitmap;
    > + p->found_pages += len;
    > + } else {
    > + return -ENOSPC;
    > + }
    > + }
    > + return 0;
    > +}
    > +
    > +static inline int export_prev_to_out(struct pagemap_scan_private *p, struct page_region __user *vec,
    > + unsigned long *vec_index)
    > +{

    No need for inline either.

    > + struct page_region *prev = &p->prev;
    > +
    > + if (prev->len) {
    > + if (copy_to_user(&vec[*vec_index], prev, sizeof(struct page_region)))
    > + return -EFAULT;
    > + p->vec_index++;
    > + (*vec_index)++;
    > + prev->len = 0;
    > + }
    > + return 0;
    > +}
    > +
    > +static inline int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long start,
    > + unsigned long end, struct mm_walk *walk)
    > +{

    Same, no need for inline. I've a few comments more in my mind will try to
    collect them tomorrow.

    \
     
     \ /
      Last update: 2023-03-27 00:15    [W:2.169 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site