linux-kernel - Re: [PATCH v5 12/24] erofs: introduce tagged pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190731130148.GE15806@quack2.suse.cz>
Date:   Wed, 31 Jul 2019 15:01:48 +0200
From:   Jan Kara <jack@...e.cz>
To:     Gao Xiang <gaoxiang25@...wei.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        Theodore Ts'o <tytso@....edu>, David Sterba <dsterba@...e.cz>,
        Amir Goldstein <amir73il@...il.com>,
        Christoph Hellwig <hch@...radead.org>,
        "Darrick J . Wong" <darrick.wong@...cle.com>,
        Dave Chinner <david@...morbit.com>,
        Jaegeuk Kim <jaegeuk@...nel.org>, Jan Kara <jack@...e.cz>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, devel@...verdev.osuosl.org,
        LKML <linux-kernel@...r.kernel.org>,
        linux-erofs@...ts.ozlabs.org, Chao Yu <yuchao0@...wei.com>,
        Miao Xie <miaoxie@...wei.com>,
        Li Guifu <bluce.liguifu@...wei.com>,
        Fang Wei <fangwei1@...wei.com>
Subject: Re: [PATCH v5 12/24] erofs: introduce tagged pointer

On Tue 30-07-19 15:14:01, Gao Xiang wrote:
> Currently kernel has scattered tagged pointer usages
> hacked by hand in plain code, without a unique and
> portable functionset to highlight the tagged pointer
> itself and wrap these hacked code in order to clean up
> all over meaningless magic masks.
> 
> This patch introduces simple generic methods to fold
> tags into a pointer integer. Currently it supports
> the last n bits of the pointer for tags, which can be
> selected by users.
> 
> In addition, it will also be used for the upcoming EROFS
> filesystem, which heavily uses tagged pointer pproach
>  to reduce extra memory allocation.
> 
> Link: https://en.wikipedia.org/wiki/Tagged_pointer
> 
> Signed-off-by: Gao Xiang <gaoxiang25@...wei.com>

I'm not sure the generic approach you take is really needed here... You can
rely on getting at most two unused bits in the pointer anyway (and on mk68
architecture I've heard even that is not true but I guess you don't care).
So why not just define a single pointer type representing pointer with as
many tags as you can get? Also what I find bad about your tagptr approach
is that the way you've implemented it you loose the information about the
original pointer type. So overall I'm not sure the benefits outweight the
downsides but I guess that's a matter of taste and ultimately your call as
a maintainer of this code.

								Honza

> ---
>  fs/erofs/tagptr.h | 110 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 110 insertions(+)
>  create mode 100644 fs/erofs/tagptr.h
> 
> diff --git a/fs/erofs/tagptr.h b/fs/erofs/tagptr.h
> new file mode 100644
> index 000000000000..a72897c86744
> --- /dev/null
> +++ b/fs/erofs/tagptr.h
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * A tagged pointer implementation
> + *
> + * Copyright (C) 2018 Gao Xiang <gaoxiang25@...wei.com>
> + */
> +#ifndef __EROFS_FS_TAGPTR_H
> +#define __EROFS_FS_TAGPTR_H
> +
> +#include <linux/types.h>
> +#include <linux/build_bug.h>
> +
> +/*
> + * the name of tagged pointer types are tagptr{1, 2, 3...}_t
> + * avoid directly using the internal structs __tagptr{1, 2, 3...}
> + */
> +#define __MAKE_TAGPTR(n) \
> +typedef struct __tagptr##n {	\
> +	uintptr_t v;	\
> +} tagptr##n##_t;
> +
> +__MAKE_TAGPTR(1)
> +__MAKE_TAGPTR(2)
> +__MAKE_TAGPTR(3)
> +__MAKE_TAGPTR(4)
> +
> +#undef __MAKE_TAGPTR
> +
> +extern void __compiletime_error("bad tagptr tags")
> +	__bad_tagptr_tags(void);
> +
> +extern void __compiletime_error("bad tagptr type")
> +	__bad_tagptr_type(void);
> +
> +/* fix the broken usage of "#define tagptr2_t tagptr3_t" by users */
> +#define __tagptr_mask_1(ptr, n)	\
> +	__builtin_types_compatible_p(typeof(ptr), struct __tagptr##n) ? \
> +		(1UL << (n)) - 1 :
> +
> +#define __tagptr_mask(ptr)	(\
> +	__tagptr_mask_1(ptr, 1) ( \
> +	__tagptr_mask_1(ptr, 2) ( \
> +	__tagptr_mask_1(ptr, 3) ( \
> +	__tagptr_mask_1(ptr, 4) ( \
> +	__bad_tagptr_type(), 0)))))
> +
> +/* generate a tagged pointer from a raw value */
> +#define tagptr_init(type, val) \
> +	((typeof(type)){ .v = (uintptr_t)(val) })
> +
> +/*
> + * directly cast a tagged pointer to the native pointer type, which
> + * could be used for backward compatibility of existing code.
> + */
> +#define tagptr_cast_ptr(tptr) ((void *)(tptr).v)
> +
> +/* encode tagged pointers */
> +#define tagptr_fold(type, ptr, _tags) ({ \
> +	const typeof(_tags) tags = (_tags); \
> +	if (__builtin_constant_p(tags) && (tags & ~__tagptr_mask(type))) \
> +		__bad_tagptr_tags(); \
> +tagptr_init(type, (uintptr_t)(ptr) | tags); })
> +
> +/* decode tagged pointers */
> +#define tagptr_unfold_ptr(tptr) \
> +	((void *)((tptr).v & ~__tagptr_mask(tptr)))
> +
> +#define tagptr_unfold_tags(tptr) \
> +	((tptr).v & __tagptr_mask(tptr))
> +
> +/* operations for the tagger pointer */
> +#define tagptr_eq(_tptr1, _tptr2) ({ \
> +	typeof(_tptr1) tptr1 = (_tptr1); \
> +	typeof(_tptr2) tptr2 = (_tptr2); \
> +	(void)(&tptr1 == &tptr2); \
> +(tptr1).v == (tptr2).v; })
> +
> +/* lock-free CAS operation */
> +#define tagptr_cmpxchg(_ptptr, _o, _n) ({ \
> +	typeof(_ptptr) ptptr = (_ptptr); \
> +	typeof(_o) o = (_o); \
> +	typeof(_n) n = (_n); \
> +	(void)(&o == &n); \
> +	(void)(&o == ptptr); \
> +tagptr_init(o, cmpxchg(&ptptr->v, o.v, n.v)); })
> +
> +/* wrap WRITE_ONCE if atomic update is needed */
> +#define tagptr_replace_tags(_ptptr, tags) ({ \
> +	typeof(_ptptr) ptptr = (_ptptr); \
> +	*ptptr = tagptr_fold(*ptptr, tagptr_unfold_ptr(*ptptr), tags); \
> +*ptptr; })
> +
> +#define tagptr_set_tags(_ptptr, _tags) ({ \
> +	typeof(_ptptr) ptptr = (_ptptr); \
> +	const typeof(_tags) tags = (_tags); \
> +	if (__builtin_constant_p(tags) && (tags & ~__tagptr_mask(*ptptr))) \
> +		__bad_tagptr_tags(); \
> +	ptptr->v |= tags; \
> +*ptptr; })
> +
> +#define tagptr_clear_tags(_ptptr, _tags) ({ \
> +	typeof(_ptptr) ptptr = (_ptptr); \
> +	const typeof(_tags) tags = (_tags); \
> +	if (__builtin_constant_p(tags) && (tags & ~__tagptr_mask(*ptptr))) \
> +		__bad_tagptr_tags(); \
> +	ptptr->v &= ~tags; \
> +*ptptr; })
> +
> +#endif	/* __EROFS_FS_TAGPTR_H */
> +
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR