| Message ID | dbfc718cd3200071765007c7ca0a2ba242181d05.1728491453.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cover-letter: iommufd: Add vIOMMU infrastructure (Part-1) | expand |
On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote: > @@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, > iommufd_object_remove(ictx, obj, obj->id, 0); > } > > -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, > - size_t size, > - enum iommufd_object_type type); > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, > + size_t size, > + enum iommufd_object_type type); Maybe call it raw instead of elm? elm suggests it is an item in an array or likewise Naming aside Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Thu, Oct 17, 2024 at 11:14:16AM -0300, Jason Gunthorpe wrote: > On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote: > > > @@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, > > iommufd_object_remove(ictx, obj, obj->id, 0); > > } > > > > -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, > > - size_t size, > > - enum iommufd_object_type type); > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, > > + size_t size, > > + enum iommufd_object_type type); > > Maybe call it raw instead of elm? elm suggests it is an item in an > array or likewise Or keep this as the __ and rename #define __iommufd_object_alloc(ictx, ptr, type, obj) \ That one to _elm like this: #define iommufd_object_alloc_elm(ictx, ptr, type, elm) \ container_of(_iommufd_object_alloc( \ ictx, \ sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ offsetof(typeof(*(ptr)), \ obj) != 0), \ type), \ typeof(*(ptr)), elm) #define iommufd_object_alloc(ictx, ptr, type) \ iommufd_object_alloc_elm(ictx, ptr, type, obj) Then you can keep the pattern of _ being the allocation function of the macro Jason
On Thu, Oct 17, 2024 at 12:37:49PM -0300, Jason Gunthorpe wrote: > On Thu, Oct 17, 2024 at 11:14:16AM -0300, Jason Gunthorpe wrote: > > On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote: > > > > > @@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, > > > iommufd_object_remove(ictx, obj, obj->id, 0); > > > } > > > > > > -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, > > > - size_t size, > > > - enum iommufd_object_type type); > > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, > > > + size_t size, > > > + enum iommufd_object_type type); > > > > Maybe call it raw instead of elm? elm suggests it is an item in an > > array or likewise > > Or keep this as the __ and rename You mean "_" v.s. "__"? > #define __iommufd_object_alloc(ictx, ptr, type, obj) \ > > That one to _elm like this: > > #define iommufd_object_alloc_elm(ictx, ptr, type, elm) \ > container_of(_iommufd_object_alloc( \ > ictx, \ > sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ > offsetof(typeof(*(ptr)), \ > obj) != 0), \ > type), \ > typeof(*(ptr)), elm) > > #define iommufd_object_alloc(ictx, ptr, type) \ > iommufd_object_alloc_elm(ictx, ptr, type, obj) > > Then you can keep the pattern of _ being the allocation function of > the macro If I get it correctly, the change would be [From] level-0: iommufd_object_alloc() level-1: __iommufd_object_alloc() level-2: _iommufd_object_alloc() [To] level-0: iommufd_object_alloc() level-1: iommufd_object_alloc_elm() level-2: _iommufd_object_alloc() i.e. change the level-1 only. Thanks Nicolin
On Thu, Oct 17, 2024 at 09:12:03AM -0700, Nicolin Chen wrote: > > Then you can keep the pattern of _ being the allocation function of > > the macro > > If I get it correctly, the change would be > [From] > level-0: iommufd_object_alloc() > level-1: __iommufd_object_alloc() > level-2: _iommufd_object_alloc() > [To] > level-0: iommufd_object_alloc() > level-1: iommufd_object_alloc_elm() > level-2: _iommufd_object_alloc() > > i.e. change the level-1 only. You could also call it _iommufd_object_alloc_elm() to keep the pattern Maymbe "member" is a better word here than elm Jason
On Thu, Oct 17, 2024 at 01:35:07PM -0300, Jason Gunthorpe wrote: > On Thu, Oct 17, 2024 at 09:12:03AM -0700, Nicolin Chen wrote: > > > > Then you can keep the pattern of _ being the allocation function of > > > the macro > > > > If I get it correctly, the change would be > > [From] > > level-0: iommufd_object_alloc() > > level-1: __iommufd_object_alloc() > > level-2: _iommufd_object_alloc() > > [To] > > level-0: iommufd_object_alloc() > > level-1: iommufd_object_alloc_elm() > > level-2: _iommufd_object_alloc() > > > > i.e. change the level-1 only. > > You could also call it _iommufd_object_alloc_elm() to keep the pattern > > Maymbe "member" is a better word here than elm Ack. level-0: iommufd_object_alloc() level-1: _iommufd_object_alloc_member() level-2: _iommufd_object_alloc() Thanks Nicolin
On Thu, Oct 17, 2024 at 09:48:23AM -0700, Nicolin Chen wrote: > On Thu, Oct 17, 2024 at 01:35:07PM -0300, Jason Gunthorpe wrote: > > On Thu, Oct 17, 2024 at 09:12:03AM -0700, Nicolin Chen wrote: > > > > > > Then you can keep the pattern of _ being the allocation function of > > > > the macro > > > > > > If I get it correctly, the change would be > > > [From] > > > level-0: iommufd_object_alloc() > > > level-1: __iommufd_object_alloc() > > > level-2: _iommufd_object_alloc() > > > [To] > > > level-0: iommufd_object_alloc() > > > level-1: iommufd_object_alloc_elm() > > > level-2: _iommufd_object_alloc() > > > > > > i.e. change the level-1 only. > > > > You could also call it _iommufd_object_alloc_elm() to keep the pattern > > > > Maymbe "member" is a better word here than elm > > Ack. > > level-0: iommufd_object_alloc() > level-1: _iommufd_object_alloc_member() > level-2: _iommufd_object_alloc() Keep the pattern: level-0: iommufd_object_alloc() level-1: iommufd_object_alloc_member() level-2: _iommufd_object_alloc_member() Jason
On 18/10/24 02:37, Jason Gunthorpe wrote: > On Thu, Oct 17, 2024 at 11:14:16AM -0300, Jason Gunthorpe wrote: >> On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote: >> >>> @@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, >>> iommufd_object_remove(ictx, obj, obj->id, 0); >>> } >>> >>> -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, >>> - size_t size, >>> - enum iommufd_object_type type); >>> +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, >>> + size_t size, >>> + enum iommufd_object_type type); >> >> Maybe call it raw instead of elm? elm suggests it is an item in an >> array or likewise > > Or keep this as the __ and rename > > #define __iommufd_object_alloc(ictx, ptr, type, obj) \ > > That one to _elm like this: > > #define iommufd_object_alloc_elm(ictx, ptr, type, elm) \ > container_of(_iommufd_object_alloc( \ > ictx, \ > sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ > offsetof(typeof(*(ptr)), \ > obj) != 0), \ > type), \ > typeof(*(ptr)), elm) > > #define iommufd_object_alloc(ictx, ptr, type) \ > iommufd_object_alloc_elm(ictx, ptr, type, obj) Bikeshedding, yay :) After starring at it for 10min - honestly - ditch iommufd_object_alloc_elm() and just pass "obj" (or "common.obj" in that single other occasion) to iommufd_object_alloc(). __iommufd_object_alloc() - a function - will the actual alloc, iommufd_object_alloc() - a macro - will do the types + call the __ variant, simple and no naming issues. And it would be real nice if it was "iobj" not this "obj" which is way too generic. Thanks, > Then you can keep the pattern of _ being the allocation function of > the macro > > Jason
On Mon, Oct 21, 2024 at 12:26:54PM +1100, Alexey Kardashevskiy wrote: > On 18/10/24 02:37, Jason Gunthorpe wrote: > > On Thu, Oct 17, 2024 at 11:14:16AM -0300, Jason Gunthorpe wrote: > > > On Wed, Oct 09, 2024 at 09:38:02AM -0700, Nicolin Chen wrote: > > > > > > > @@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, > > > > iommufd_object_remove(ictx, obj, obj->id, 0); > > > > } > > > > > > > > -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, > > > > - size_t size, > > > > - enum iommufd_object_type type); > > > > +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, > > > > + size_t size, > > > > + enum iommufd_object_type type); > > > > > > Maybe call it raw instead of elm? elm suggests it is an item in an > > > array or likewise > > > > Or keep this as the __ and rename > > > > #define __iommufd_object_alloc(ictx, ptr, type, obj) \ > > > > That one to _elm like this: > > > > #define iommufd_object_alloc_elm(ictx, ptr, type, elm) \ > > container_of(_iommufd_object_alloc( \ > > ictx, \ > > sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ > > offsetof(typeof(*(ptr)), \ > > obj) != 0), \ > > type), \ > > typeof(*(ptr)), elm) > > > > #define iommufd_object_alloc(ictx, ptr, type) \ > > iommufd_object_alloc_elm(ictx, ptr, type, obj) > > > Bikeshedding, yay :) > > After starring at it for 10min - honestly - ditch > iommufd_object_alloc_elm() and just pass "obj" (or "common.obj" in that > single other occasion) to iommufd_object_alloc(). > > __iommufd_object_alloc() - a function - will the actual alloc, > iommufd_object_alloc() - a macro - will do the types + call the __ > variant, simple and no naming issues. All three-level helpers have callers. So that would be a bigger patch than I expected to include in this series. Maybe I should just drop this patch, since it's not functionally necessary. If we want to clean the whole thing, can do with a separate series. > And it would be real nice if it was "iobj" not this "obj" which is way > too generic. Thanks, Again, the renaming would be across the whole folder, not only here. So, I think it could be a separate cleanup series later. Thanks Nicolin
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index 7221f81ae211..f2f3a906eac9 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -217,12 +217,12 @@ iommufd_object_put_and_try_destroy(struct iommufd_ctx *ictx, iommufd_object_remove(ictx, obj, obj->id, 0); } -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, - size_t size, - enum iommufd_object_type type); +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, + size_t size, + enum iommufd_object_type type); #define __iommufd_object_alloc(ictx, ptr, type, obj) \ - container_of(_iommufd_object_alloc( \ + container_of(iommufd_object_alloc_elm( \ ictx, \ sizeof(*(ptr)) + BUILD_BUG_ON_ZERO( \ offsetof(typeof(*(ptr)), \ diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index b5f5d27ee963..28e1ef5666e9 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -29,9 +29,9 @@ struct iommufd_object_ops { static const struct iommufd_object_ops iommufd_object_ops[]; static struct miscdevice vfio_misc_dev; -struct iommufd_object *_iommufd_object_alloc(struct iommufd_ctx *ictx, - size_t size, - enum iommufd_object_type type) +struct iommufd_object *iommufd_object_alloc_elm(struct iommufd_ctx *ictx, + size_t size, + enum iommufd_object_type type) { struct iommufd_object *obj; int rc;
Currently, the object allocation function calls: level-0: iommufd_object_alloc() level-1: __iommufd_object_alloc() level-2: _iommufd_object_alloc() So the level-1 and level-2 look inverted. The level-1 allocator is a container_of converter with a pointer sanity, backing the level-0 allocator. But the level-2 allocator does the actual object element allocations. Thus, rename the level-2 allocator, so those two inverted namings would be: level-0: iommufd_object_alloc() level-1: __iommufd_object_alloc() level-2: iommufd_object_alloc_elm() Suggested-by: Jason Gunthorpe <jgg@nvidia.com> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/iommufd/iommufd_private.h | 8 ++++---- drivers/iommu/iommufd/main.c | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-)