| Message ID | 20190730071413.11871-13-gaoxiang25@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | erofs: promote erofs from staging | expand |
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@huawei.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@huawei.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 >
Hi Jan, On 2019/7/31 21:01, Jan Kara wrote: > 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@huawei.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). Yes, and currently erofs uses 1-bit tags at most... > So why not just define a single pointer type representing pointer with as > many tags as you can get? I think the primary use is to decide if the tag is beyond the bit boundary, such as use tag 2, 3 on tagptr1_t, we can BUG_ON or check it at compile time.... BTW, my first patch is the only one fixed tagged pointer type(2-bit even if m64k) as below: https://lore.kernel.org/lkml/1530176789-107541-1-git-send-email-gaoxiang25@huawei.com/ and Willy raised another problem is about static variable, therefore I decided to leave multiple tagptr types for users to decide for specific situations... https://lore.kernel.org/lkml/20180628092303.GD7646@bombadil.infradead.org/ > 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. Yes, I think that is about coding style, but the legacy way we have to do type cast as well, I think... struct b *ptr = tagptr_unfold_tags(tptr); vs struct b *ptr = (struct b *)((unsigned long)tptr & ~2); > 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. I think I wouldn't generalize this implementations in this series... It will be used for EROFS only for now :) Thanks, Gao Xiang > > 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@huawei.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 >>
On 2019/7/31 21:20, Gao Xiang wrote: > Yes, I think that is about coding style, but the legacy way we have to do > type cast as well, I think... > > struct b *ptr = tagptr_unfold_tags(tptr); > vs > struct b *ptr = (struct b *)((unsigned long)tptr & ~2); and we could do "typedef tagptr1_t tptrb;" and then use tptrb for tagged pointer rather than barely use tagptr1_t tagptr2_t ... as I mentioned in: https://lore.kernel.org/lkml/0c2cdd4f-8fe7-6084-9c2d-c2e475e6806e@aol.com/ I think "tptrb" is enough for developers to know the original pointer type when they coding... OTOH, I think it could be better not to directly use "struct b *" to represent the whole tagged pointer since it seems unsafe to do dereference directly.. It could introduce some potential bugs... All in all, this approach is only used for EROFS for now... If there are some better implementation, I can switch to it in the later version :) Thanks, Gao Xiang
On 2019/7/31 21:20, Gao Xiang wrote: > struct b *ptr = tagptr_unfold_tags(tptr); > vs > struct b *ptr = (struct b *)((unsigned long)tptr & ~2); Sorry ... a too stupid typo issue, I mean.... struct b *ptr = tagptr_unfold_ptr(tptr); vs struct b *ptr = (struct b *)((unsigned long)tptr & ~3); Thanks, Gao Xiang
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@huawei.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 */ +
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@huawei.com> --- fs/erofs/tagptr.h | 110 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 fs/erofs/tagptr.h