Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Jul 2023 16:53:46 +0300
From:   Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To:     Alexander Potapenko <glider@google.com>
Cc:     catalin.marinas@arm.com, will@kernel.org, pcc@google.com,
        andreyknvl@gmail.com, linux@rasmusvillemoes.dk,
        yury.norov@gmail.com, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, eugenis@google.com,
        syednwaris@gmail.com, william.gray@linaro.org
Subject: Re: [PATCH v3 5/5] arm64: mte: add compression support to mteswap.c
Message-ID: <ZLVH6t25HD+HhCka@smile.fi.intel.com>
References: <20230717113709.328671-1-glider@google.com>
 <20230717113709.328671-6-glider@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230717113709.328671-6-glider@google.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Mon, Jul 17, 2023 at 01:37:08PM +0200, Alexander Potapenko wrote:
> Define the internal mteswap.h interface:
>  - _mte_alloc_and_save_tags()
>  - _mte_free_saved_tags()
>  - _mte_restore_tags()
> 
> , that encapsulates saving tags for a struct page (together with memory
> allocation), restoring tags, and deleting the storage allocated for them.
> 
> These functions accept opaque pointers, which may point to 128-byte
> tag buffers, as well as smaller buffers containing compressed tags, or
> have compressed tags stored directly in them.
> 
> The existing code from mteswap.c operating with uncompressed tags is split
> away into mteswap_nocomp.c, and the newly introduced mteswap_comp.c
> provides compression with the EA0 algorithm. The latter implementation
> is picked if CONFIG_ARM64_MTE_COMP=y.
> 
> Soon after booting Android, tag compression saves ~2.5x memory previously
> spent by mteswap.c on tag allocations. With the growing uptime, the
> savings reach 20x and even more.

...

> +#ifndef ARCH_ARM64_MM_MTESWAP_H_
> +#define ARCH_ARM64_MM_MTESWAP_H_

> +#include <linux/mm_types.h>

But you actually don't use that.

struct page;

forward declaration is 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;

> +	size = ea0_storage_size(handle);
> +	ea0_release_handle(handle);
> +}

> +void _mte_restore_tags(void *tags, struct page *page)
> +{

As per above.

> +	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;

> +}

...

> +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.

> +}

-- 
With Best Regards,
Andy Shevchenko