Re: [PATCH v8 3/3] arm64: mte: add compression support to mteswap.c

[Alexander Potapenko]

> >
> > If CONFIG_ARM64_MTE_COMP is enabled, mteswap.c will attemt to compress
>
> attempt?

Good catch!

> >
> >   # cat /sys/kernel/debug/mteswap/stats
> >   8 bytes: 102496 allocations, 67302 deallocations
> >   128 bytes: 212234 allocations, 178278 deallocations
> >   uncompressed tag storage size: 8851200
> >   compressed tag storage size: 4346368
>
> Can you align them like this:
>
>    # cat /sys/kernel/debug/mteswap/stats
>    8 bytes:      102496 allocations,    67302 deallocations
>    128 bytes:    212234 allocations,   178278 deallocations
>    uncompressed tag storage size:    8851200
>    compressed tag storage size:      4346368

Ok, will do in v9.

> And also, can you mention a new file in the documentation?

Sure.

> IIRC, it was my suggestion to measure some stats... If so, can you add:
>
> Suggested-by: Yury Norov <yury.norov@xxxxxxxxx> # for stats

Will do.


> > +static atomic_long_t alloc_counters[MTESWAP_CTR_SIZE];
> > +static atomic_long_t dealloc_counters[MTESWAP_CTR_SIZE];
>
> I think it's worth to add them and all the book keeping code in
> a separate patch? Also can you consider making them configurable,
> maybe depending on CONFIG_ARM64_MTE_SWAP_STAT?..

Sounds fine, I'll split this code into a separate patch.

> >  static DEFINE_XARRAY(mte_pages);
> >
> >  void *mte_allocate_tag_storage(void)
> >  {
> > -     /* tags granule is 16 bytes, 2 tags stored per byte */
> > -     return kmalloc(MTE_PAGE_TAG_STORAGE, GFP_KERNEL);
> > +     void *ret;
> > +
> > +     ret = kmalloc(MTE_PAGE_TAG_STORAGE, GFP_KERNEL);
> > +     if (ret)
> > +             atomic_long_inc(&alloc_counters[MTESWAP_CTR_NOINLINE]);
> > +     return ret;
> >  }
> >
> >  void mte_free_tag_storage(char *storage)
>
> If you use a term 'free' here, the counter name should probably
> reflect that.

I still want to keep the terms "allocation/deallocation" in the
user-facing code (in the /sys/ file) though.
So renaming the counter here will cause a mismatch between its name
and the stats output.

>
> >  {
> > -     kfree(storage);
> > +     if (!mte_is_compressed(storage)) {
> > +             kfree(storage);
> > +             atomic_long_dec(&alloc_counters[MTESWAP_CTR_NOINLINE]);
>
> s/NOINLINE/OUTLINE ?

Done.

> >       mte_save_page_tags(page_address(page), tag_storage);
> > +     compressed = mte_compress(tag_storage);
> > +     if (compressed) {
> > +             mte_free_tag_storage(tag_storage);
> > +             tag_storage = (void *)compressed;
>
> But 'compressed' is already 'void *', what for typecasting?

Yeah, it used to be unsigned long or something like that. Removed the cast.

> Also, it's a bad naming - adjective implies bool type. I'd name it
> 'compressed_tag', or similar.
>
> > +             atomic_long_inc(&alloc_counters[MTESWAP_CTR_INLINE]);
> > +     }
>
> Is it possible to move all this conditional inside the function call?
> I feel like it should be a single-line:
>
>         tag_storage = mte_compress(tag_storage);
>
> So that client code will be free of housekeeping details.

The problem is that this assignment destroys the current value of
tag_storage, which means mte_compress() will have to deal with memory
deallocation.
That, in turn, will introduce an unnecessary dependency between
mtecomp.c and mteswap.c (so that the former can call
mte_free_tag_storage()).
Given that we still have to deal with allocations in mteswap.c, I
don't think it's worth the hassle.

>
> >
> >       /* lookup the swap entry.val from the page */
> >       ret = xa_store(&mte_pages, page_swap_entry(page).val, tag_storage,
> > @@ -50,13 +76,20 @@ int mte_save_tags(struct page *page)
> >  void mte_restore_tags(swp_entry_t entry, struct page *page)
> >  {
> >       void *tags = xa_load(&mte_pages, entry.val);
> > +     void *tag_storage = NULL;
> >
> >       if (!tags)
> >               return;
> >
> >       if (try_page_mte_tagging(page)) {
> > +             if (mte_is_compressed(tags)) {
> > +                     tag_storage = mte_allocate_tag_storage();
> > +                     mte_decompress(tags, tag_storage);
> > +                     tags = tag_storage;
> > +             }
>
> Same here, if it's possible, I'd prefer a single line call instead of
> the above chunk:
>
>         tags = mte_decompress(tags);
>         if (!tags)
>                 return;

Same here, we'd have to make mte_decompress() allocate/deallocate
memory in a way consistent with mteswap.c allocations.