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.
