On Tue, Jan 31, 2023 at 09:16:45PM -0800, Jiaqi Yan wrote:
> > > > > +/*
> > > > > + * Copies memory with #MC in source page (@from) handled. Returns number
> > > > > + * of bytes not copied if there was an exception; otherwise 0 for success.
> > > > > + * Note handling #MC requires arch opt-in.
> > > > > + */
> > > > > +static int copy_mc_page(struct page *to, struct page *from)
> > > > > +{
> > > > > + char *vfrom, *vto;
> > > > > + unsigned long ret;
> > > > > +
> > > > > + vfrom = kmap_local_page(from);
> > > > > + vto = kmap_local_page(to);
> > > > > + ret = copy_mc_to_kernel(vto, vfrom, PAGE_SIZE);
> > > > > + if (ret == 0)
> > > > > + kmsan_copy_page_meta(to, from);
> > > > > + kunmap_local(vto);
> > > > > + kunmap_local(vfrom);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > >
> > > >
> > > > It is very similar to copy_mc_user_highpage(), but uses
> > > > kmsan_copy_page_meta() instead of kmsan_unpoison_memory().
> > > >
> > > > Could you explain the difference? I don't quite get it.
> > >
> > > copy_mc_page is actually the MC version of copy_highpage, which uses
> > > kmsan_copy_page_meta instead of kmsan_unpoison_memory.
> > >
> > > My understanding is kmsan_copy_page_meta covers kmsan_unpoison_memory.
> > > When there is no metadata (kmsan_shadow or kmsan_origin), both
> > > kmsan_copy_page_meta and kmsan_unpoison_memory just do
> > > kmsan_internal_unpoison_memory to mark the memory range as
> > > initialized; when there is metadata in src page, kmsan_copy_page_meta
> > > will copy whatever metadata in src to dst. So I think
> > > kmsan_copy_page_meta is the right thing to do.
> >
> > Should we fix copy_mc_user_highpage() then?
>
> I think it depends on what copy_user_highpage() (the original of
> copy_mc_user_highpage) is used for. copy_mc_user_highpage is currently
> only used by __wp_page_copy_user, is it possible that here we don't
> want to (or don't need to) copy page metadata for userspace pages?
Tony, could chime in on this? Can we modify copy_mc_user_highpage() to
also use kmsan_copy_page_meta()? I don't really understand KMSAN here.
>
> >
> > > > Indentation levels get out of control. Maybe some code restructuring is
> > > > required?
> > >
> > > v10 will change to something like this to reduce 1 level of indentation:
> > >
> > > if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval)))
> > > continue;
> > > src_page = pte_page(pteval);
> > > if (!PageCompound(src_page))
> > > release_pte_page(src_page);
> >
> > I hoped for deeper rework. Maybe split the function into several functions
> > and make overall structure more readable?
>
> How about turning the 2nd loop into
> __collapse_huge_page_copy_succeeded and
> __collapse_huge_page_copy_failed, one for the case copy succeeded, and
> one for failed? Like this:
> if (likely(result == SCAN_SUCCEED))
> __collapse_huge_page_copy_succeeded(...);
> else
> __collapse_huge_page_copy_failed(...);
>
> My prototype shows it could reduce the level indents.
Give it a try and try to get into reader shoes. Get it easily digestible
for someone who reads the code for the first time.
--
Kiryl Shutsemau / Kirill A. Shutemov
