Messages in this thread | | | From | Alexander Potapenko <> | Date | Wed, 8 Nov 2023 14:53:29 +0100 | Subject | Re: [PATCH v8 3/3] arm64: mte: add compression support to mteswap.c |
| |
> > > > 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@gmail.com> # 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.
| |