Re: [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.