Re: [PATCH v3 05/19] xen/arm: mm: Add support for the contiguous bit

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 2 Apr 2022, Julien Grall wrote:
> On 02/04/2022 00:53, Stefano Stabellini wrote:
> > On Mon, 21 Feb 2022, Julien Grall wrote:
> > > @@ -1333,21 +1386,34 @@ static int xen_pt_update(unsigned long virt,
> > >       while ( left )
> > >       {
> > >           unsigned int order, level;
> > > +        unsigned int nr_contig;
> > > +        unsigned int new_flags;
> > >             level = xen_pt_mapping_level(vfn, mfn, left, flags);
> > >           order = XEN_PT_LEVEL_ORDER(level);
> > >             ASSERT(left >= BIT(order, UL));
> > >   -        rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, level,
> > > flags);
> > > -        if ( rc )
> > > -            break;
> > > +        /*
> > > +         * Check if we can set the contiguous mapping and update the
> > > +         * flags accordingly.
> > > +         */
> > > +        nr_contig = xen_pt_check_contig(vfn, mfn, level, left, flags);
> > > +        new_flags = flags | ((nr_contig > 1) ? _PAGE_CONTIG : 0);
> > 
> > Here is an optional idea to make the code simpler. We could move the
> > flags changes (adding/removing _PAGE_CONTIG) to xen_pt_check_contig.
> > That way, we could remove the inner loop.
> > 
> > xen_pt_check_contig could check if _PAGE_CONTIG is already set and based
> > on alignment, it should be able to figure out when it needs to be
> > disabled.
> 
> My initial attempt was to do everything in a loop. But this didn't pan out as
> I wanted (I felt the code was complex) and there are extra work to be done for
> the next 31 entries (assuming 4KB granularity).
> 
> Hence the two loops. Unfortunately, I didn't keep my first attempt. So I can't
> realy show what I wrote.

I trusted you that the resulting code with a single loop was worse.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>