Re: [PATCH v3 5/5] arm64: mte: add compression support to mteswap.c

[Alexander Potapenko]

> > +#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!