Re: [PATCH v10 3/6] fs/proc/task_mmu: Implement IOCTL to get and/or the clear info about PTEs

[Cyrill Gorcunov]

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.