Hello Andrei,
Thank you for reviewing.
On 11/10/22 4:54 AM, Andrei Vagin wrote: [...]
+static int add_to_out(bool sd, bool file, bool pres, bool swap, struct pagemap_scan_private *p,
unsigned long addr, unsigned int len)+{
- unsigned long bitmap, cur = sd | file << 1 | pres << 2 | swap << 3;
Should we define contants for each of these bits?
I think I can define a macro to hide this dirty bit shifting in the function.
- bool cpy = true;
- 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) {
if ((p->vec_index) && (p->vec[p->vec_index - 1].bitmap == bitmap) &&(p->vec[p->vec_index - 1].start + p->vec[p->vec_index - 1].len * PAGE_SIZE ==addr)) {I think it is better to define a variable for p->vec_index - 1.
Will do in the next revision.
nit: len can be in bytes rather than pages.
We are considering memory in the page units. The memory given to this IOCTL must have PAGE_SIZE alignment. Oterwise we error out (picked this from mincore()).
+static int pagemap_scan_pmd_entry(pmd_t *pmd, unsigned long addr,
unsigned long end, struct mm_walk *walk)+{
- struct pagemap_scan_private *p = walk->private;
- struct vm_area_struct *vma = walk->vma;
- unsigned int len;
- spinlock_t *ptl;
- int ret = 0;
- pte_t *pte;
- bool dirty_vma = (p->flags & PAGEMAP_NO_REUSED_REGIONS) ?
(false) : (vma->vm_flags & VM_SOFTDIRTY);- if ((walk->vma->vm_end < addr) || (p->max_pages && p->found_pages == p->max_pages))
return 0;- end = min(end, walk->vma->vm_end);
- ptl = pmd_trans_huge_lock(pmd, vma);
- if (ptl) {
if (dirty_vma || check_soft_dirty_pmd(vma, addr, pmd, false)) {/** Break huge page into small pages if operation needs to be performed is* on a portion of the huge page or the return buffer cannot store complete* data.*/if ((IS_CLEAR_OP(p) && (end - addr < HPAGE_SIZE))) {spin_unlock(ptl);split_huge_pmd(vma, pmd, addr);goto process_smaller_pages;}if (IS_GET_OP(p)) {len = (end - addr)/PAGE_SIZE;if (p->max_pages && p->found_pages + len > p->max_pages)len = p->max_pages - p->found_pages;ret = add_to_out(dirty_vma ||check_soft_dirty_pmd(vma, addr, pmd, false),can we reuse a return code of the previous call of check_soft_dirty_pmd?
Yes, will do.
vma->vm_file, pmd_present(*pmd), is_swap_pmd(*pmd),p, addr, len);}if (!ret && IS_CLEAR_OP(p))check_soft_dirty_pmd(vma, addr, pmd, true);should we return a error in this case? We need to be sure that:
- we stop waking page tables after this point.
I'll update the implementation to return error. It immediately terminates the walk as well.
- return this error to the user-space if we are not able to add anything in the vector.
I'm not returning error to userspace if we found no page matching the masks. The total number of filled page_region are returned from the IOCTL. If IOCTL returns 0, it means no page found has found, but the IOCTL executed successfully.
[...]
+static long do_pagemap_sd_cmd(struct mm_struct *mm, struct pagemap_scan_arg *arg) +{
- struct mmu_notifier_range range;
- unsigned long __user start, end;
- struct pagemap_scan_private p;
- int ret;
- start = (unsigned long)untagged_addr(arg->start);
- if ((!IS_ALIGNED(start, PAGE_SIZE)) || (!access_ok((void __user *)start, arg->len)))
return -EINVAL;- if (IS_GET_OP(arg) &&
((arg->vec_len == 0) || (!access_ok((struct page_region *)arg->vec, arg->vec_len))))return -ENOMEM;+#ifndef CONFIG_MEM_SOFT_DIRTY
- if (IS_SD_OP(arg) || (arg->required_mask & PAGE_IS_SOFTDIRTY) ||
(arg->anyof_mask & PAGE_IS_SOFTDIRTY))return -EINVAL;+#endif
- if ((arg->flags & ~PAGEMAP_SD_FLAGS) || (arg->required_mask & ~PAGEMAP_OP_MASK) ||
(arg->anyof_mask & ~PAGEMAP_OP_MASK) || (arg->excluded_mask & ~PAGEMAP_OP_MASK) ||(arg->return_mask & ~PAGEMAP_OP_MASK))return -EINVAL;- if ((!arg->required_mask && !arg->anyof_mask && !arg->excluded_mask) || !arg->return_mask)
return -EINVAL;- if (IS_SD_OP(arg) && ((arg->required_mask & PAGEMAP_NONSD_OP_MASK) ||
(arg->anyof_mask & PAGEMAP_NONSD_OP_MASK)))return -EINVAL;- end = start + arg->len;
- p.max_pages = arg->max_pages;
- p.found_pages = 0;
- p.flags = arg->flags;
- p.required_mask = arg->required_mask;
- p.anyof_mask = arg->anyof_mask;
- p.excluded_mask = arg->excluded_mask;
- p.return_mask = arg->return_mask;
- p.vec_index = 0;
- p.vec_len = arg->vec_len;
- if (IS_GET_OP(arg)) {
p.vec = vzalloc(arg->vec_len * sizeof(struct page_region));I think we need to set a reasonable limit for vec_len to avoid large allocations on the kernel. We can consider to use kmalloc or kvmalloc here.
I'll update to kvzalloc which uses vmalloc if kmalloc fails. It'll use kmalloc for smaller allocations. Thanks for suggesting it. But it'll not limit the memory allocation.
Thanks, Andrei