Re: [PATCH v1 2/5] xen/riscv: implement maddr_to_virt()

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Fri Oct 18, 2024 at 2:17 PM BST, oleksii.kurochko wrote:
> On Thu, 2024-10-17 at 16:55 +0200, Jan Beulich wrote:
> > On 16.10.2024 11:15, Oleksii Kurochko wrote:
> > > --- a/xen/arch/riscv/include/asm/mm.h
> > > +++ b/xen/arch/riscv/include/asm/mm.h
> > > @@ -25,8 +25,12 @@
> > >  
> > >  static inline void *maddr_to_virt(paddr_t ma)
> > >  {
> > > -    BUG_ON("unimplemented");
> > > -    return NULL;
> > > +    /* Offset in the direct map, accounting for pdx compression */
> > > +    unsigned long va_offset = maddr_to_directmapoff(ma);
> > 
> > Why the mentioning of PDX compression?
> It was mentioned because if PDX will be enabled maddr_to_directmapoff()
> will take into account PDX stuff.
>
> >  At least right now it's unavailable
> > for RISC-V afaics. Are there plans to change that any time soon?
> At the moment, I don't have such plans, looking at available platform
> there are no a lot of benefits of having PDX compression now.
>
> Perhaps it would be good to add
> BUILD_BUG_ON(IS_ENABLED(PDX_COMPRESSION)) for the places which should
> be updated when CONFIG_PDX will be enabled.
>
> ~ Oleksii

I'd just forget about it unless you ever notice you're wasting a lot of entries
in the frame table due to empty space in the memory map. Julien measured the
effect on Amazon's Live Migration as a 10% improvement in downtime with PDX
off.

PDX compression shines when you have separate RAM banks at very, very
disparately far addresses (specifics in pdx.h). Unfortunately the flip side of
this compression is that you get several memory accesses for each single
pdx-(to/from)-mfn conversion. And we do a lot of those. One possible solution
would be to alt-patch the values in the code-stream and avoid the perf-hit, but
that's not merged. Jan had some patches but that didn't make it to staging,
IIRC.

Cheers,
Alejandro