[PATCH v3 5/5] arm64: mte: add compression support to mteswap.c
Alexander Potapenko
glider at google.com
Tue Jul 18 03:48:00 PDT 2023
> > +#include <linux/mm_types.h>
>
> But you actually don't use that.
>
> struct page;
>
> forward declaration is enough.
Fair enough.
>
> > +void *_mte_alloc_and_save_tags(struct page *page);
> > +void _mte_free_saved_tags(void *tags);
> > +void _mte_restore_tags(void *tags, struct page *page);
> > +
> > +#endif // ARCH_ARM64_MM_MTESWAP_H_
>
> ...
>
> > +void _mte_free_saved_tags(void *storage)
> > +{
> > + unsigned long handle = xa_to_value(storage);
> > + int size;
> > +
> > + if (!handle)
> > + return;
>
> Perhaps
>
> unsigned long handle;
>
> handle = xa_to_value(storage);
> if (!handle)
> return;
I don't have a strong preference and am happy to change this, but, out
of curiosity, why do you think it is better?
This pattern (calling (even non-)trivial functions when declaring
variables) is widely used across the kernel.
Or is it just for consistency with how `handle` is used in the rest of the file?
> > +void _mte_restore_tags(void *tags, struct page *page)
> > +{
>
> As per above.
Ack
> > + if (try_page_mte_tagging(page)) {
> > + if (!ea0_decompress(handle, tags_decomp))
> > + return;
> > + mte_restore_page_tags(page_address(page), tags_decomp);
> > + set_page_mte_tagged(page);
> > + }
>
> I think you may drop an indentation level by
>
> if (!try_page_mte_tagging(page))
> return;
>
> > +}
Ack
> ...
>
> > +void _mte_restore_tags(void *tags, struct page *page)
> > +{
> > + if (try_page_mte_tagging(page)) {
> > + mte_restore_page_tags(page_address(page), tags);
> > + set_page_mte_tagged(page);
> > + }
>
> Ditto.
Thanks!
More information about the linux-arm-kernel
mailing list