| Message ID | CAAKD3BAYMpjb8H16Bc2kw+Pj5s5=h=BgwqO6uP4xh-FpOgyFhA@mail.gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
> -----Original Message----- > From: Matan Barak [mailto:matanb@dev.mellanox.co.il] > Sent: Sunday, March 11, 2018 12:35 PM > To: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Ruhl, Michael J <michael.j.ruhl@intel.com>; Leon Romanovsky > <leon@kernel.org>; Doug Ledford <dledford@redhat.com>; Leon > Romanovsky <leonro@mellanox.com>; RDMA mailing list <linux- > rdma@vger.kernel.org>; Aviad Yehezkel <aviadye@mellanox.com>; Boris > Pismenny <borisp@mellanox.com>; Matan Barak <matanb@mellanox.com>; > Yishai Hadas <yishaih@mellanox.com> > Subject: Re: [PATCH rdma-next 02/12] IB/uverbs: Move to new headers and > make naming consistent > > On Thu, Mar 8, 2018 at 11:45 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Thu, Mar 08, 2018 at 09:16:32PM +0000, Ruhl, Michael J wrote: > >> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- > >> > owner@vger.kernel.org] On Behalf Of Leon Romanovsky > >> > Sent: Thursday, March 8, 2018 12:19 PM > >> > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe > >> > <jgg@mellanox.com> > >> > Cc: Leon Romanovsky <leonro@mellanox.com>; RDMA mailing list > <linux- > >> > rdma@vger.kernel.org>; Aviad Yehezkel <aviadye@mellanox.com>; > Boris > >> > Pismenny <borisp@mellanox.com>; Matan Barak > <matanb@mellanox.com>; > >> > Yishai Hadas <yishaih@mellanox.com> > >> > Subject: [PATCH rdma-next 02/12] IB/uverbs: Move to new headers and > >> > make naming consistent > >> > > >> > From: Matan Barak <matanb@mellanox.com> > >> > > >> > Use macros to make names consistent in ioctl() uAPI: > >> > The ioctl() uAPI works with object-method hierarchy. The method part > >> > also states which handler should be executed when this method is > called > >> > from user-space. Therefore, we need to tie method, method's id, > method's > >> > handler and the object owning this method together. > >> > Previously, this was done through explicit developer chosen names. > >> > This makes grepping the code harder. Changing the method's name, > >> > method's handler and object's name to be automatically generated > based > >> > on the ids. > >> > > >> > The headers are split in a way so they be included and used by > >> > user-space. One header strictly contains structures that are used > >> > directly by user-space applications, where another header is used for > >> > internal library (i.e. libibverbs) to form the ioctl() commands. > >> > Other header simply contains the required general command structure. > >> > > >> > Reviewed-by: Yishai Hadas <yishaih@mellanox.com> > >> > Signed-off-by: Matan Barak <matanb@mellanox.com> > >> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> > >> > drivers/infiniband/core/uverbs_std_types.c | 139 ++++++++++++-- > ------ > >> > > >> > diff --git a/drivers/infiniband/core/uverbs_std_types.c > >> > b/drivers/infiniband/core/uverbs_std_types.c > >> > index df1360e6774f..99f971b6cc67 100644 > >> > +++ b/drivers/infiniband/core/uverbs_std_types.c > >> > @@ -210,15 +210,24 @@ static int > >> > uverbs_hot_unplug_completion_event_file(struct ib_uobject_file > *uobj_ > >> > return 0; > >> > }; > >> > > >> > +#define UVERBS_METHOD(id) uverbs_method_##id > >> > +#define UVERBS_HANDLER(id) uverbs_handler_##id > >> > + > >> > +#define DECLARE_COMMON_METHOD(id, ...) \ > >> > + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, > >> > UVERBS_HANDLER(id), ##__VA_ARGS__) > >> > + > >> > +#define DECLARE_COMMON_OBJECT(id, ...) \ > >> > + DECLARE_UVERBS_OBJECT(UVERBS_OBJECT(id), id, > >> > ##__VA_ARGS__) > >> > >> I applied this patch to my tree and noticed that the declarations for the > >> METHODS and OBJECTS and HANDLERs had changed. > >> > >> I assumed that the above macros had been updated in the appropriate > >> header files. When I tried to apply these "new" macros to my tree, I > couldn't > >> compile. > >> > >> So I cannot (easily) use the example of uverbs_std_verbs.c to set up my > own > >> stuff, and if I do, I have to copy the above macros to my files, or decipher > these > >> macros to figure out what I should use. > > > > Yeah, the normal DECLARE_UVERBS_METHOD/etc should just do this, we > shouldn't > > wrapper them in uvrebs_std_types.c > > > > Ok, so this is the proposed change: > > diff --git a/include/rdma/uverbs_named_ioctl.h > b/include/rdma/uverbs_named_ioctl.h > new file mode 100644 > index 000000000000..f9dff179a740 > --- /dev/null > +++ b/include/rdma/uverbs_named_ioctl.h > @@ -0,0 +1,59 @@ > +/* > + * Copyright (c) 2018, Mellanox Technologies inc. All rights reserved. > + * > + * This software is available to you under a choice of one of two > + * licenses. You may choose to be licensed under the terms of the GNU > + * General Public License (GPL) Version 2, available from the file > + * COPYING in the main directory of this source tree, or the > + * OpenIB.org BSD license below: > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * - Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the following > + * disclaimer. > + * > + * - Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES > OF > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT > HOLDERS > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN > AN > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR > IN > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN > THE > + * SOFTWARE. > + */ > + > +#ifndef _UVERBS_NAMED_IOCTL_ > +#define _UVERBS_NAMED_IOCTL_ > + > +#include <rdma/uverbs_ioctl.h> > + > +#ifndef UVERBS_MODULE_NAME > +#error "Please #define UVERBS_MODULE_NAME before including > rdma/uverbs_named_ioctl.h" > +#endif > + > +#define _UVERBS_PASTE(x, y) x ## y > +#define _UVERBS_NAME(x, y) _UVERBS_PASTE(x, y) > +#define UVERBS_METHOD(id) > _UVERBS_NAME(UVERBS_MODULE_NAME, _method_##id) > +#define UVERBS_HANDLER(id) > _UVERBS_NAME(UVERBS_MODULE_NAME, _handler_##id) > + > +#define DECLARE_NAMED_METHOD(id, ...) \ > + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, > UVERBS_HANDLER(id), ##__VA_ARGS__) > + > +#define DECLARE_NAMED_METHOD_WITH_HANDLER(id, handler, ...) \ > + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, handler, > ##__VA_ARGS__) > + > +#define DECLARE_NAMED_METHOD_NO_HANDLER(id, handler, ...) \ Should there be a handler in this parameter list? > + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, NULL, > ##__VA_ARGS__) > + > + > +#define DECLARE_NAMED_OBJECT(id, ...) \ > + DECLARE_UVERBS_OBJECT(UVERBS_OBJECT(id), id, ##__VA_ARGS__) > + > +#endif > > In order to use this layer of macros, you'll have to define your > module name (UVERBS_MODULE_NAME). > The other names follow automatically. I am still trying to understand the mechanism that will allow me to "extend" standard objects in a driver specific way (i.e. if I want to add new methods to a standard object that are driver specific). I am assuming that I will include a declaration of the standard object in the driver specific code, and then add the driver specific methods to it, and then the spec_merge function will do its magic to put everything in place. If this is the correct way to do this, then I would use the "unnamed" version to specify the standard object in the drive code (and have to unraveled the macro in the standard area to get the derived name)? Or am I misunderstanding this feature? > I first tried to use KBUILD_MODNAME, but since it's a stringifyed > version of our module name, it doesn't work :( > > Later on I intend to try adding here some easy-to-use macros to > declare a parsing tree with one argument (that extends one of the > common methods). Is this easy-to-use macro going to "fix" my comment above? Thanks, Mike > > Are you ok with that? > > > Jason > > Matan > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 13, 2018 at 3:38 PM, Ruhl, Michael J <michael.j.ruhl@intel.com> wrote: >> -----Original Message----- >> From: Matan Barak [mailto:matanb@dev.mellanox.co.il] >> Sent: Sunday, March 11, 2018 12:35 PM >> To: Jason Gunthorpe <jgg@ziepe.ca> >> Cc: Ruhl, Michael J <michael.j.ruhl@intel.com>; Leon Romanovsky >> <leon@kernel.org>; Doug Ledford <dledford@redhat.com>; Leon >> Romanovsky <leonro@mellanox.com>; RDMA mailing list <linux- >> rdma@vger.kernel.org>; Aviad Yehezkel <aviadye@mellanox.com>; Boris >> Pismenny <borisp@mellanox.com>; Matan Barak <matanb@mellanox.com>; >> Yishai Hadas <yishaih@mellanox.com> >> Subject: Re: [PATCH rdma-next 02/12] IB/uverbs: Move to new headers and >> make naming consistent >> >> On Thu, Mar 8, 2018 at 11:45 PM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >> > On Thu, Mar 08, 2018 at 09:16:32PM +0000, Ruhl, Michael J wrote: >> >> > From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma- >> >> > owner@vger.kernel.org] On Behalf Of Leon Romanovsky >> >> > Sent: Thursday, March 8, 2018 12:19 PM >> >> > To: Doug Ledford <dledford@redhat.com>; Jason Gunthorpe >> >> > <jgg@mellanox.com> >> >> > Cc: Leon Romanovsky <leonro@mellanox.com>; RDMA mailing list >> <linux- >> >> > rdma@vger.kernel.org>; Aviad Yehezkel <aviadye@mellanox.com>; >> Boris >> >> > Pismenny <borisp@mellanox.com>; Matan Barak >> <matanb@mellanox.com>; >> >> > Yishai Hadas <yishaih@mellanox.com> >> >> > Subject: [PATCH rdma-next 02/12] IB/uverbs: Move to new headers and >> >> > make naming consistent >> >> > >> >> > From: Matan Barak <matanb@mellanox.com> >> >> > >> >> > Use macros to make names consistent in ioctl() uAPI: >> >> > The ioctl() uAPI works with object-method hierarchy. The method part >> >> > also states which handler should be executed when this method is >> called >> >> > from user-space. Therefore, we need to tie method, method's id, >> method's >> >> > handler and the object owning this method together. >> >> > Previously, this was done through explicit developer chosen names. >> >> > This makes grepping the code harder. Changing the method's name, >> >> > method's handler and object's name to be automatically generated >> based >> >> > on the ids. >> >> > >> >> > The headers are split in a way so they be included and used by >> >> > user-space. One header strictly contains structures that are used >> >> > directly by user-space applications, where another header is used for >> >> > internal library (i.e. libibverbs) to form the ioctl() commands. >> >> > Other header simply contains the required general command structure. >> >> > >> >> > Reviewed-by: Yishai Hadas <yishaih@mellanox.com> >> >> > Signed-off-by: Matan Barak <matanb@mellanox.com> >> >> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com> >> >> > drivers/infiniband/core/uverbs_std_types.c | 139 ++++++++++++-- >> ------ >> >> > >> >> > diff --git a/drivers/infiniband/core/uverbs_std_types.c >> >> > b/drivers/infiniband/core/uverbs_std_types.c >> >> > index df1360e6774f..99f971b6cc67 100644 >> >> > +++ b/drivers/infiniband/core/uverbs_std_types.c >> >> > @@ -210,15 +210,24 @@ static int >> >> > uverbs_hot_unplug_completion_event_file(struct ib_uobject_file >> *uobj_ >> >> > return 0; >> >> > }; >> >> > >> >> > +#define UVERBS_METHOD(id) uverbs_method_##id >> >> > +#define UVERBS_HANDLER(id) uverbs_handler_##id >> >> > + >> >> > +#define DECLARE_COMMON_METHOD(id, ...) \ >> >> > + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, >> >> > UVERBS_HANDLER(id), ##__VA_ARGS__) >> >> > + >> >> > +#define DECLARE_COMMON_OBJECT(id, ...) \ >> >> > + DECLARE_UVERBS_OBJECT(UVERBS_OBJECT(id), id, >> >> > ##__VA_ARGS__) >> >> >> >> I applied this patch to my tree and noticed that the declarations for the >> >> METHODS and OBJECTS and HANDLERs had changed. >> >> >> >> I assumed that the above macros had been updated in the appropriate >> >> header files. When I tried to apply these "new" macros to my tree, I >> couldn't >> >> compile. >> >> >> >> So I cannot (easily) use the example of uverbs_std_verbs.c to set up my >> own >> >> stuff, and if I do, I have to copy the above macros to my files, or decipher >> these >> >> macros to figure out what I should use. >> > >> > Yeah, the normal DECLARE_UVERBS_METHOD/etc should just do this, we >> shouldn't >> > wrapper them in uvrebs_std_types.c >> > >> >> Ok, so this is the proposed change: >> >> diff --git a/include/rdma/uverbs_named_ioctl.h >> b/include/rdma/uverbs_named_ioctl.h >> new file mode 100644 >> index 000000000000..f9dff179a740 >> --- /dev/null >> +++ b/include/rdma/uverbs_named_ioctl.h >> @@ -0,0 +1,59 @@ >> +/* >> + * Copyright (c) 2018, Mellanox Technologies inc. All rights reserved. >> + * >> + * This software is available to you under a choice of one of two >> + * licenses. You may choose to be licensed under the terms of the GNU >> + * General Public License (GPL) Version 2, available from the file >> + * COPYING in the main directory of this source tree, or the >> + * OpenIB.org BSD license below: >> + * >> + * Redistribution and use in source and binary forms, with or >> + * without modification, are permitted provided that the following >> + * conditions are met: >> + * >> + * - Redistributions of source code must retain the above >> + * copyright notice, this list of conditions and the following >> + * disclaimer. >> + * >> + * - Redistributions in binary form must reproduce the above >> + * copyright notice, this list of conditions and the following >> + * disclaimer in the documentation and/or other materials >> + * provided with the distribution. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES >> OF >> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND >> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT >> HOLDERS >> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN >> AN >> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR >> IN >> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN >> THE >> + * SOFTWARE. >> + */ >> + >> +#ifndef _UVERBS_NAMED_IOCTL_ >> +#define _UVERBS_NAMED_IOCTL_ >> + >> +#include <rdma/uverbs_ioctl.h> >> + >> +#ifndef UVERBS_MODULE_NAME >> +#error "Please #define UVERBS_MODULE_NAME before including >> rdma/uverbs_named_ioctl.h" >> +#endif >> + >> +#define _UVERBS_PASTE(x, y) x ## y >> +#define _UVERBS_NAME(x, y) _UVERBS_PASTE(x, y) >> +#define UVERBS_METHOD(id) >> _UVERBS_NAME(UVERBS_MODULE_NAME, _method_##id) >> +#define UVERBS_HANDLER(id) >> _UVERBS_NAME(UVERBS_MODULE_NAME, _handler_##id) >> + >> +#define DECLARE_NAMED_METHOD(id, ...) \ >> + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, >> UVERBS_HANDLER(id), ##__VA_ARGS__) >> + >> +#define DECLARE_NAMED_METHOD_WITH_HANDLER(id, handler, ...) \ >> + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, handler, >> ##__VA_ARGS__) >> + >> +#define DECLARE_NAMED_METHOD_NO_HANDLER(id, handler, ...) \ > > Should there be a handler in this parameter list? > This is for the cases where the provider driver just want to add attributes to a common method. Previously, this was done by extending the ib_udata given to the provider driver callbacks. However, this makes it harder to extend and put more burden on the provider driver to validate. I wanted to use the standard parser to validate everything, that's why I preferred this approach. Therefore, when you call merge_trees(T1, T2), you'll get the handler of the last tree which is != NULL. >> + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, NULL, >> ##__VA_ARGS__) >> + >> + >> +#define DECLARE_NAMED_OBJECT(id, ...) \ >> + DECLARE_UVERBS_OBJECT(UVERBS_OBJECT(id), id, ##__VA_ARGS__) >> + >> +#endif >> >> In order to use this layer of macros, you'll have to define your >> module name (UVERBS_MODULE_NAME). >> The other names follow automatically. > > I am still trying to understand the mechanism that will allow me to "extend" > standard objects in a driver specific way (i.e. if I want to add new methods > to a standard object that are driver specific). > > I am assuming that I will include a declaration of the standard object in the > driver specific code, and then add the driver specific methods to it, and then > the spec_merge function will do its magic to put everything in place. > That's correct. > If this is the correct way to do this, then I would use the "unnamed" version > to specify the standard object in the drive code (and have to unraveled the > macro in the standard area to get the derived name)? > > Or am I misunderstanding this feature? > You can use the named one. You just have to make sure the id(s) are the same. The name parameter is only used for the macros reference. >> I first tried to use KBUILD_MODNAME, but since it's a stringifyed >> version of our module name, it doesn't work :( >> >> Later on I intend to try adding here some easy-to-use macros to >> declare a parsing tree with one argument (that extends one of the >> common methods). > > Is this easy-to-use macro going to "fix" my comment above? > Yeah, I just declared another UVERBS_MODULE_NAME macro that takes allows you to use the named version. > Thanks, > > Mike > >> Regards, Matan >> Are you ok with that? >> >> > Jason >> >> Matan >> >> > -- >> > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >> > the body of a message to majordomo@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/rdma/uverbs_named_ioctl.h b/include/rdma/uverbs_named_ioctl.h new file mode 100644 index 000000000000..f9dff179a740 --- /dev/null +++ b/include/rdma/uverbs_named_ioctl.h @@ -0,0 +1,59 @@ +/* + * Copyright (c) 2018, Mellanox Technologies inc. All rights reserved. + * + * This software is available to you under a choice of one of two + * licenses. You may choose to be licensed under the terms of the GNU + * General Public License (GPL) Version 2, available from the file + * COPYING in the main directory of this source tree, or the + * OpenIB.org BSD license below: + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above + * copyright notice, this list of conditions and the following + * disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials + * provided with the distribution. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#ifndef _UVERBS_NAMED_IOCTL_ +#define _UVERBS_NAMED_IOCTL_ + +#include <rdma/uverbs_ioctl.h> + +#ifndef UVERBS_MODULE_NAME +#error "Please #define UVERBS_MODULE_NAME before including rdma/uverbs_named_ioctl.h" +#endif + +#define _UVERBS_PASTE(x, y) x ## y +#define _UVERBS_NAME(x, y) _UVERBS_PASTE(x, y) +#define UVERBS_METHOD(id) _UVERBS_NAME(UVERBS_MODULE_NAME, _method_##id) +#define UVERBS_HANDLER(id) _UVERBS_NAME(UVERBS_MODULE_NAME, _handler_##id) + +#define DECLARE_NAMED_METHOD(id, ...) \ + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, UVERBS_HANDLER(id), ##__VA_ARGS__) + +#define DECLARE_NAMED_METHOD_WITH_HANDLER(id, handler, ...) \ + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, handler, ##__VA_ARGS__) + +#define DECLARE_NAMED_METHOD_NO_HANDLER(id, handler, ...) \ + DECLARE_UVERBS_METHOD(UVERBS_METHOD(id), id, NULL, ##__VA_ARGS__) + + +#define DECLARE_NAMED_OBJECT(id, ...) \