| Message ID | 1565eca3cf1a3344cd0de450140ddb3a2a89ccc4.1547757100.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | IWPM support for no port mapping | expand |
Hi Steve, Your port mapper changes look good. I just have some minor comments. It appears the port mapping will fail, if the kernel port mapper is updated with your changes but the user space port mapper isn't (ABI version 3). Is there a reason why the error is silent in this case? > static const char iwpm_ulib_name[IWPM_ULIBNAME_SIZE] = > "iWarpPortMapperUser"; -static int iwpm_ulib_version = 3; > +u16 iwpm_user_ulib_version = IWPM_UABI_VERSION_MIN; Rename iwpm_user_ulib_version to iwpm_ulib_version > + > +int iwpm_send_hello(u8 nl_client, int iwpm_pid, u16 abi_version) { > + struct sk_buff *skb = NULL; > + struct nlmsghdr *nlh; > + u32 msg_seq; Remove msg_seq, because it isn't used in the function > +/* iwarp port mapper message flags */ > +enum { > + > + /* Do not map the port for this IWPM request */ > + IWPM_FLAGS_NO_PORT_MAP = (1 << 0), Use BIT(0) for consistency within the patch Thank you, Tatyana
> -----Original Message----- > From: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com> > Sent: Friday, January 25, 2019 6:32 PM > To: Steve Wise <swise@opengridcomputing.com>; dledford@redhat.com; > jgg@mellanox.com > Cc: linux-rdma@vger.kernel.org; BMT@zurich.ibm.com; Saleem, Shiraz > <shiraz.saleem@intel.com> > Subject: RE: [PATCH RESEND v1 rdma-next 3/3] RDMA/IWPM: Support no > port mapping requirements > > Hi Steve, > > Your port mapper changes look good. I just have some minor comments. > Thanks for reviewing! > It appears the port mapping will fail, if the kernel port mapper is updated > with your changes but the user space port mapper isn't (ABI version 3). Is > there a reason why the error is silent in this case? > With testing, I see it works with kernel ABI4 and user ABI3. How do you see it failing exactly? Maybe I'm missing something. > > static const char iwpm_ulib_name[IWPM_ULIBNAME_SIZE] = > > "iWarpPortMapperUser"; -static int iwpm_ulib_version = 3; > > +u16 iwpm_user_ulib_version = IWPM_UABI_VERSION_MIN; > > Rename iwpm_user_ulib_version to iwpm_ulib_version > Seems good. > > + > > +int iwpm_send_hello(u8 nl_client, int iwpm_pid, u16 abi_version) { > > + struct sk_buff *skb = NULL; > > + struct nlmsghdr *nlh; > > + u32 msg_seq; > > Remove msg_seq, because it isn't used in the function > Yes. > > +/* iwarp port mapper message flags */ > > +enum { > > + > > + /* Do not map the port for this IWPM request */ > > + IWPM_FLAGS_NO_PORT_MAP = (1 << 0), > > Use BIT(0) for consistency within the patch > I've seen other maintainers suggest BIT() is obfuscating. Jason?
> > Hi Steve, > > > > Your port mapper changes look good. I just have some minor comments. > > > > Thanks for reviewing! > > > It appears the port mapping will fail, if the kernel port mapper is > updated > > with your changes but the user space port mapper isn't (ABI version > > 3). Is there a reason why the error is silent in this case? > > > > With testing, I see it works with kernel ABI4 and user ABI3. How do you > see it failing exactly? Maybe I'm missing something. > I am referring to the following case, which anticipates mismatch between the kernel and user space port mapper: > + /* If flags are required and we're not V4, then return a quite error */ > + if (pm_msg->flags && iwpm_user_ulib_version == > IWPM_UABI_VERSION_MIN) { > + ret = -EINVAL; > + goto query_mapping_error_nowarn; > + } > + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { > + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, > + IWPM_NLA_QUERY_FLAGS); > + if (ret) > + goto query_mapping_error; > + } > > > +/* iwarp port mapper message flags */ enum { > > > + > > > + /* Do not map the port for this IWPM request */ > > > + IWPM_FLAGS_NO_PORT_MAP = (1 << 0), > > > > Use BIT(0) for consistency within the patch > > > > I've seen other maintainers suggest BIT() is obfuscating. Jason? > In another place in the patch you are using the BIT() macro, so my comment here is just a matter of consistency. I don't have a strong opinion on which approach you choose. > +enum iw_flags { > + > + /* > + * This flag allows the iwcm and iwpmd to still advertise > + * mappings but the real and mapped port numbers are the > + * same. Further, iwpmd will not bind any user socket to > + * reserve the port. This is required for soft iwarp > + * to play in the port mapped iwarp space. > + */ > + IW_F_NO_PORT_MAP = BIT(0), > +}; > +
> -----Original Message----- > From: Nikolova, Tatyana E <tatyana.e.nikolova@intel.com> > Sent: Friday, January 25, 2019 9:58 PM > To: Steve Wise <swise@opengridcomputing.com>; dledford@redhat.com; > jgg@mellanox.com > Cc: linux-rdma@vger.kernel.org; BMT@zurich.ibm.com; Saleem, Shiraz > <shiraz.saleem@intel.com>; 'Jason Gunthorpe' <jgg@mellanox.com> > Subject: RE: [PATCH RESEND v1 rdma-next 3/3] RDMA/IWPM: Support no > port mapping requirements > > > > > Hi Steve, > > > > > > Your port mapper changes look good. I just have some minor comments. > > > > > > > Thanks for reviewing! > > > > > It appears the port mapping will fail, if the kernel port mapper is > > updated > > > with your changes but the user space port mapper isn't (ABI version > > > 3). Is there a reason why the error is silent in this case? > > > > > > > With testing, I see it works with kernel ABI4 and user ABI3. How do you > > see it failing exactly? Maybe I'm missing something. > > > > I am referring to the following case, which anticipates mismatch between the > kernel and user space port mapper: > > > + /* If flags are required and we're not V4, then return a quite error */ > > + if (pm_msg->flags && iwpm_user_ulib_version == > > IWPM_UABI_VERSION_MIN) { > > + ret = -EINVAL; > > + goto query_mapping_error_nowarn; > > + } > > + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { > > + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, > > + IWPM_NLA_QUERY_FLAGS); > > + if (ret) > > + goto query_mapping_error; > > + } > Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp drivers will work fine because they aren't trying to use the new NO_PORT_MAP flag in pm_msg->flags. If, however, you have, say the soft iwarp driver running, then pm_msg->flags would be non zero. If this is the case, then the V4 kernel must not send up the V4 FLAGS nlattr because the V3 iwpmd can't handle it. So the above code fails the map request and this results in no port mapping done for the soft iwarp driver. So we get backward compatibility with existing iwarp drivers. But new drivers that need NO_PORT_MAP will only work in the port mapping game if their local iwpmd is V4. > > > > > +/* iwarp port mapper message flags */ enum { > > > > + > > > > + /* Do not map the port for this IWPM request */ > > > > + IWPM_FLAGS_NO_PORT_MAP = (1 << 0), > > > > > > Use BIT(0) for consistency within the patch > > > > > > > I've seen other maintainers suggest BIT() is obfuscating. Jason? > > > > In another place in the patch you are using the BIT() macro, so my comment > here is just a matter of consistency. > I don't have a strong opinion on which approach you choose. > I should make it consistent one way or the other. Thanks, Steve.
> > > > It appears the port mapping will fail, if the kernel port mapper is updated > > > > with your changes but the user space port mapper isn't (ABI > > > > version 3). Is there a reason why the error is silent in this case? > > > > > > > > > > With testing, I see it works with kernel ABI4 and user ABI3. How do you > > > see it failing exactly? Maybe I'm missing something. > > > > > > > I am referring to the following case, which anticipates mismatch > > between the kernel and user space port mapper: > > > > > + /* If flags are required and we're not V4, then return a quite > > > +error > */ > > > + if (pm_msg->flags && iwpm_user_ulib_version == > > > IWPM_UABI_VERSION_MIN) { > > > + ret = -EINVAL; > > > + goto query_mapping_error_nowarn; > > > + } > > > + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { > > > + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, > > > + IWPM_NLA_QUERY_FLAGS); > > > + if (ret) > > > + goto query_mapping_error; > > > + } > > > > Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp drivers > will work fine because they aren't trying to use the new NO_PORT_MAP flag > in pm_msg->flags. > > If, however, you have, say the soft iwarp driver running, then pm_msg->flags > would be non zero. If this is the case, then the V4 kernel must not send up > the V4 FLAGS nlattr because the V3 iwpmd can't handle it. > So the above code fails the map request and this results in no port mapping > done for the soft iwarp driver. > > So we get backward compatibility with existing iwarp drivers. But new drivers > that need NO_PORT_MAP will only work in the port mapping game if their > local iwpmd is V4. > Hi Steve, Thank you for the explanation. I think that you don't need to make the error silent in this case. You could go through the standard error path, where the error is logged and people are going to know about it. This is going to prompt them to update to a more current rdma-core which has your changes. I understand that the error doesn't matter in the case of soft iwarp devices talking to each other, since they don't need a port mapper. However, in the case of a soft iwarp device talking to an iwarp peer which uses the port mapper, the error reporting is going to be useful, since the connection establishment won't succeed. Regards, Tatyana
On 1/27/2019 10:58 AM, Nikolova, Tatyana E wrote: > >>>>> It appears the port mapping will fail, if the kernel port mapper is updated >>>>> with your changes but the user space port mapper isn't (ABI >>>>> version 3). Is there a reason why the error is silent in this case? >>>>> >>>> With testing, I see it works with kernel ABI4 and user ABI3. How do you >>>> see it failing exactly? Maybe I'm missing something. >>>> >>> I am referring to the following case, which anticipates mismatch >>> between the kernel and user space port mapper: >>> >>>> + /* If flags are required and we're not V4, then return a quite >>>> +error >> */ >>>> + if (pm_msg->flags && iwpm_user_ulib_version == >>>> IWPM_UABI_VERSION_MIN) { >>>> + ret = -EINVAL; >>>> + goto query_mapping_error_nowarn; >>>> + } >>>> + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { >>>> + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, >>>> + IWPM_NLA_QUERY_FLAGS); >>>> + if (ret) >>>> + goto query_mapping_error; >>>> + } >> Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp drivers >> will work fine because they aren't trying to use the new NO_PORT_MAP flag >> in pm_msg->flags. >> >> If, however, you have, say the soft iwarp driver running, then pm_msg->flags >> would be non zero. If this is the case, then the V4 kernel must not send up >> the V4 FLAGS nlattr because the V3 iwpmd can't handle it. >> So the above code fails the map request and this results in no port mapping >> done for the soft iwarp driver. >> >> So we get backward compatibility with existing iwarp drivers. But new drivers >> that need NO_PORT_MAP will only work in the port mapping game if their >> local iwpmd is V4. >> > Hi Steve, > > Thank you for the explanation. > > I think that you don't need to make the error silent in this case. You could go through the standard error path, where the error is logged and people are going to know about it. This is going to prompt them to update to a more current rdma-core which has your changes. > > I understand that the error doesn't matter in the case of soft iwarp devices talking to each other, since they don't need a port mapper. However, in the case of a soft iwarp device talking to an iwarp peer which uses the port mapper, the error reporting is going to be useful, since the connection establishment won't succeed. > > Regards, > Tatyana That makes sense. But I only want a warn_once() type of warning. Thanks for reviewing this. I'll respin it soon. May I add your reviewed-by tag? Steve.
On Mon, Jan 28, 2019 at 10:22:26AM -0600, Steve Wise wrote: > > On 1/27/2019 10:58 AM, Nikolova, Tatyana E wrote: > > > >>>>> It appears the port mapping will fail, if the kernel port mapper is updated > >>>>> with your changes but the user space port mapper isn't (ABI > >>>>> version 3). Is there a reason why the error is silent in this case? > >>>>> > >>>> With testing, I see it works with kernel ABI4 and user ABI3. How do you > >>>> see it failing exactly? Maybe I'm missing something. > >>>> > >>> I am referring to the following case, which anticipates mismatch > >>> between the kernel and user space port mapper: > >>> > >>>> + /* If flags are required and we're not V4, then return a quite > >>>> +error > >> */ > >>>> + if (pm_msg->flags && iwpm_user_ulib_version == > >>>> IWPM_UABI_VERSION_MIN) { > >>>> + ret = -EINVAL; > >>>> + goto query_mapping_error_nowarn; > >>>> + } > >>>> + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { > >>>> + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, > >>>> + IWPM_NLA_QUERY_FLAGS); > >>>> + if (ret) > >>>> + goto query_mapping_error; > >>>> + } > >> Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp drivers > >> will work fine because they aren't trying to use the new NO_PORT_MAP flag > >> in pm_msg->flags. > >> > >> If, however, you have, say the soft iwarp driver running, then pm_msg->flags > >> would be non zero. If this is the case, then the V4 kernel must not send up > >> the V4 FLAGS nlattr because the V3 iwpmd can't handle it. > >> So the above code fails the map request and this results in no port mapping > >> done for the soft iwarp driver. > >> > >> So we get backward compatibility with existing iwarp drivers. But new drivers > >> that need NO_PORT_MAP will only work in the port mapping game if their > >> local iwpmd is V4. > >> > > Hi Steve, > > > > Thank you for the explanation. > > > > I think that you don't need to make the error silent in this case. You could go through the standard error path, where the error is logged and people are going to know about it. This is going to prompt them to update to a more current rdma-core which has your changes. > > > > I understand that the error doesn't matter in the case of soft iwarp devices talking to each other, since they don't need a port mapper. However, in the case of a soft iwarp device talking to an iwarp peer which uses the port mapper, the error reporting is going to be useful, since the connection establishment won't succeed. > > > > Regards, > > Tatyana > > That makes sense. But I only want a warn_once() type of warning. > Thanks for reviewing this. I'll respin it soon. dev_warn_once and print the pid/process name of the offending process is the usual way to do this. Also, please send the rdma-core patches, they need to come before the kernel patch to the user api is applied Jason
On 1/28/2019 11:34 AM, Jason Gunthorpe wrote: > On Mon, Jan 28, 2019 at 10:22:26AM -0600, Steve Wise wrote: >> On 1/27/2019 10:58 AM, Nikolova, Tatyana E wrote: >>>>>>> It appears the port mapping will fail, if the kernel port mapper is updated >>>>>>> with your changes but the user space port mapper isn't (ABI >>>>>>> version 3). Is there a reason why the error is silent in this case? >>>>>>> >>>>>> With testing, I see it works with kernel ABI4 and user ABI3. How do you >>>>>> see it failing exactly? Maybe I'm missing something. >>>>>> >>>>> I am referring to the following case, which anticipates mismatch >>>>> between the kernel and user space port mapper: >>>>> >>>>>> + /* If flags are required and we're not V4, then return a quite >>>>>> +error >>>> */ >>>>>> + if (pm_msg->flags && iwpm_user_ulib_version == >>>>>> IWPM_UABI_VERSION_MIN) { >>>>>> + ret = -EINVAL; >>>>>> + goto query_mapping_error_nowarn; >>>>>> + } >>>>>> + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { >>>>>> + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, >>>>>> + IWPM_NLA_QUERY_FLAGS); >>>>>> + if (ret) >>>>>> + goto query_mapping_error; >>>>>> + } >>>> Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp drivers >>>> will work fine because they aren't trying to use the new NO_PORT_MAP flag >>>> in pm_msg->flags. >>>> >>>> If, however, you have, say the soft iwarp driver running, then pm_msg->flags >>>> would be non zero. If this is the case, then the V4 kernel must not send up >>>> the V4 FLAGS nlattr because the V3 iwpmd can't handle it. >>>> So the above code fails the map request and this results in no port mapping >>>> done for the soft iwarp driver. >>>> >>>> So we get backward compatibility with existing iwarp drivers. But new drivers >>>> that need NO_PORT_MAP will only work in the port mapping game if their >>>> local iwpmd is V4. >>>> >>> Hi Steve, >>> >>> Thank you for the explanation. >>> >>> I think that you don't need to make the error silent in this case. You could go through the standard error path, where the error is logged and people are going to know about it. This is going to prompt them to update to a more current rdma-core which has your changes. >>> >>> I understand that the error doesn't matter in the case of soft iwarp devices talking to each other, since they don't need a port mapper. However, in the case of a soft iwarp device talking to an iwarp peer which uses the port mapper, the error reporting is going to be useful, since the connection establishment won't succeed. >>> >>> Regards, >>> Tatyana >> That makes sense. But I only want a warn_once() type of warning. >> Thanks for reviewing this. I'll respin it soon. > dev_warn_once and print the pid/process name of the offending process > is the usual way to do this. Ok. > Also, please send the rdma-core patches, they need to come before the > kernel patch to the user api is applied > > Jason Will do. Steve.
> On 1/27/2019 10:58 AM, Nikolova, Tatyana E wrote: > > > >>>>> It appears the port mapping will fail, if the kernel port mapper > >>>>> is updated with your changes but the user space port mapper isn't > >>>>> (ABI version 3). Is there a reason why the error is silent in this case? > >>>>> > >>>> With testing, I see it works with kernel ABI4 and user ABI3. How do you > >>>> see it failing exactly? Maybe I'm missing something. > >>>> > >>> I am referring to the following case, which anticipates mismatch > >>> between the kernel and user space port mapper: > >>> > >>>> + /* If flags are required and we're not V4, then return a quite > >>>> +error > >> */ > >>>> + if (pm_msg->flags && iwpm_user_ulib_version == > >>>> IWPM_UABI_VERSION_MIN) { > >>>> + ret = -EINVAL; > >>>> + goto query_mapping_error_nowarn; > >>>> + } > >>>> + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { > >>>> + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, > >>>> + IWPM_NLA_QUERY_FLAGS); > >>>> + if (ret) > >>>> + goto query_mapping_error; > >>>> + } > >> Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp > >> drivers will work fine because they aren't trying to use the new > >> NO_PORT_MAP flag in pm_msg->flags. > >> > >> If, however, you have, say the soft iwarp driver running, then > >> pm_msg->flags would be non zero. If this is the case, then the V4 > >> kernel must not send up the V4 FLAGS nlattr because the V3 iwpmd can't > handle it. > >> So the above code fails the map request and this results in no port > >> mapping done for the soft iwarp driver. > >> > >> So we get backward compatibility with existing iwarp drivers. But > >> new drivers that need NO_PORT_MAP will only work in the port mapping > >> game if their local iwpmd is V4. > >> > > Hi Steve, > > > > Thank you for the explanation. > > > > I think that you don't need to make the error silent in this case. You could go > through the standard error path, where the error is logged and people are > going to know about it. This is going to prompt them to update to a more > current rdma-core which has your changes. > > > > I understand that the error doesn't matter in the case of soft iwarp devices > talking to each other, since they don't need a port mapper. However, in the > case of a soft iwarp device talking to an iwarp peer which uses the port > mapper, the error reporting is going to be useful, since the connection > establishment won't succeed. > > > > Regards, > > Tatyana > > That makes sense. But I only want a warn_once() type of warning. Thanks for > reviewing this. I'll respin it soon. > > May I add your reviewed-by tag? Sure. Thanks! > > Steve.
On 1/28/2019 11:34 AM, Jason Gunthorpe wrote: > On Mon, Jan 28, 2019 at 10:22:26AM -0600, Steve Wise wrote: >> On 1/27/2019 10:58 AM, Nikolova, Tatyana E wrote: >>>>>>> It appears the port mapping will fail, if the kernel port mapper is updated >>>>>>> with your changes but the user space port mapper isn't (ABI >>>>>>> version 3). Is there a reason why the error is silent in this case? >>>>>>> >>>>>> With testing, I see it works with kernel ABI4 and user ABI3. How do you >>>>>> see it failing exactly? Maybe I'm missing something. >>>>>> >>>>> I am referring to the following case, which anticipates mismatch >>>>> between the kernel and user space port mapper: >>>>> >>>>>> + /* If flags are required and we're not V4, then return a quite >>>>>> +error >>>> */ >>>>>> + if (pm_msg->flags && iwpm_user_ulib_version == >>>>>> IWPM_UABI_VERSION_MIN) { >>>>>> + ret = -EINVAL; >>>>>> + goto query_mapping_error_nowarn; >>>>>> + } >>>>>> + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { >>>>>> + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, >>>>>> + IWPM_NLA_QUERY_FLAGS); >>>>>> + if (ret) >>>>>> + goto query_mapping_error; >>>>>> + } >>>> Right. So if you have a V4 kernel and a V3 iwpmd, then current iwarp drivers >>>> will work fine because they aren't trying to use the new NO_PORT_MAP flag >>>> in pm_msg->flags. >>>> >>>> If, however, you have, say the soft iwarp driver running, then pm_msg->flags >>>> would be non zero. If this is the case, then the V4 kernel must not send up >>>> the V4 FLAGS nlattr because the V3 iwpmd can't handle it. >>>> So the above code fails the map request and this results in no port mapping >>>> done for the soft iwarp driver. >>>> >>>> So we get backward compatibility with existing iwarp drivers. But new drivers >>>> that need NO_PORT_MAP will only work in the port mapping game if their >>>> local iwpmd is V4. >>>> >>> Hi Steve, >>> >>> Thank you for the explanation. >>> >>> I think that you don't need to make the error silent in this case. You could go through the standard error path, where the error is logged and people are going to know about it. This is going to prompt them to update to a more current rdma-core which has your changes. >>> >>> I understand that the error doesn't matter in the case of soft iwarp devices talking to each other, since they don't need a port mapper. However, in the case of a soft iwarp device talking to an iwarp peer which uses the port mapper, the error reporting is going to be useful, since the connection establishment won't succeed. >>> >>> Regards, >>> Tatyana >> That makes sense. But I only want a warn_once() type of warning. >> Thanks for reviewing this. I'll respin it soon. > dev_warn_once and print the pid/process name of the offending process > is the usual way to do this. There is no device pointer available when this issue is detected, so I don't think dev_warn_once() is appropriate. Also, iwpm_register_pid_cb() already logs a warning for a down level iwpmd. And since the first thing iwcm does is send a REGISTER_PID to iwpmd, I don't think we need a warning in the add_mapping and add_and_query_mapping functions. There should always be one log entry if the iwpmd is down level and iwcm tries to map something. I'll re-verify this with testing. Steve. > Also, please send the rdma-core patches, they need to come before the > kernel patch to the user api is applied > > Jason
diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c index 476abc74178e..350ea2bab78a 100644 --- a/drivers/infiniband/core/iwcm.c +++ b/drivers/infiniband/core/iwcm.c @@ -87,7 +87,8 @@ const char *__attribute_const__ iwcm_reject_msg(int reason) [RDMA_NL_IWPM_REMOTE_INFO] = {.dump = iwpm_remote_info_cb}, [RDMA_NL_IWPM_HANDLE_ERR] = {.dump = iwpm_mapping_error_cb}, [RDMA_NL_IWPM_MAPINFO] = {.dump = iwpm_mapping_info_cb}, - [RDMA_NL_IWPM_MAPINFO_NUM] = {.dump = iwpm_ack_mapping_info_cb} + [RDMA_NL_IWPM_MAPINFO_NUM] = {.dump = iwpm_ack_mapping_info_cb}, + [RDMA_NL_IWPM_HELLO] = {.dump = iwpm_hello_cb} }; static struct workqueue_struct *iwcm_wq; @@ -525,6 +526,8 @@ static int iw_cm_map(struct iw_cm_id *cm_id, bool active) cm_id->mapped = true; pm_msg.loc_addr = cm_id->local_addr; pm_msg.rem_addr = cm_id->remote_addr; + pm_msg.flags = (cm_id->device->iwcm->driver_flags & IW_F_NO_PORT_MAP) ? + IWPM_FLAGS_NO_PORT_MAP : 0; if (active) status = iwpm_add_and_query_mapping(&pm_msg, RDMA_NL_IWCM); @@ -543,7 +546,7 @@ static int iw_cm_map(struct iw_cm_id *cm_id, bool active) return iwpm_create_mapinfo(&cm_id->local_addr, &cm_id->m_local_addr, - RDMA_NL_IWCM); + RDMA_NL_IWCM, pm_msg.flags); } /* diff --git a/drivers/infiniband/core/iwpm_msg.c b/drivers/infiniband/core/iwpm_msg.c index 3a8753595c8f..3f8c09224057 100644 --- a/drivers/infiniband/core/iwpm_msg.c +++ b/drivers/infiniband/core/iwpm_msg.c @@ -34,7 +34,7 @@ #include "iwpm_util.h" static const char iwpm_ulib_name[IWPM_ULIBNAME_SIZE] = "iWarpPortMapperUser"; -static int iwpm_ulib_version = 3; +u16 iwpm_user_ulib_version = IWPM_UABI_VERSION_MIN; static int iwpm_user_pid = IWPM_PID_UNDEFINED; static atomic_t echo_nlmsg_seq; @@ -130,6 +130,7 @@ int iwpm_register_pid(struct iwpm_dev_data *pm_msg, u8 nl_client) * nlmsg attributes: * [IWPM_NLA_MANAGE_MAPPING_SEQ] * [IWPM_NLA_MANAGE_ADDR] + * [IWPM_NLA_MANAGE_FLAGS] */ int iwpm_add_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) { @@ -173,6 +174,18 @@ int iwpm_add_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) if (ret) goto add_mapping_error; + /* If flags are required and we're not V4, then return a quiet error */ + if (pm_msg->flags && iwpm_user_ulib_version == IWPM_UABI_VERSION_MIN) { + ret = -EINVAL; + goto add_mapping_error_nowarn; + } + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, + IWPM_NLA_MANAGE_FLAGS); + if (ret) + goto add_mapping_error; + } + nlmsg_end(skb, nlh); nlmsg_request->req_buffer = pm_msg; @@ -187,6 +200,7 @@ int iwpm_add_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) return ret; add_mapping_error: pr_info("%s: %s (client = %d)\n", __func__, err_str, nl_client); +add_mapping_error_nowarn: if (skb) dev_kfree_skb(skb); if (nlmsg_request) @@ -201,6 +215,7 @@ int iwpm_add_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) * [IWPM_NLA_QUERY_MAPPING_SEQ] * [IWPM_NLA_QUERY_LOCAL_ADDR] * [IWPM_NLA_QUERY_REMOTE_ADDR] + * [IWPM_NLA_QUERY_FLAGS] */ int iwpm_add_and_query_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) { @@ -251,6 +266,18 @@ int iwpm_add_and_query_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) if (ret) goto query_mapping_error; + /* If flags are required and we're not V4, then return a quite error */ + if (pm_msg->flags && iwpm_user_ulib_version == IWPM_UABI_VERSION_MIN) { + ret = -EINVAL; + goto query_mapping_error_nowarn; + } + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { + ret = ibnl_put_attr(skb, nlh, sizeof(u32), &pm_msg->flags, + IWPM_NLA_QUERY_FLAGS); + if (ret) + goto query_mapping_error; + } + nlmsg_end(skb, nlh); nlmsg_request->req_buffer = pm_msg; @@ -264,6 +291,7 @@ int iwpm_add_and_query_mapping(struct iwpm_sa_data *pm_msg, u8 nl_client) return ret; query_mapping_error: pr_info("%s: %s (client = %d)\n", __func__, err_str, nl_client); +query_mapping_error_nowarn: if (skb) dev_kfree_skb(skb); if (nlmsg_request) @@ -379,7 +407,7 @@ int iwpm_register_pid_cb(struct sk_buff *skb, struct netlink_callback *cb) /* check device name, ulib name and version */ if (strcmp(pm_msg->dev_name, dev_name) || strcmp(iwpm_ulib_name, iwpm_name) || - iwpm_version != iwpm_ulib_version) { + iwpm_version < IWPM_UABI_VERSION_MIN) { pr_info("%s: Incorrect info (dev = %s name = %s version = %d)\n", __func__, dev_name, iwpm_name, iwpm_version); @@ -387,6 +415,10 @@ int iwpm_register_pid_cb(struct sk_buff *skb, struct netlink_callback *cb) goto register_pid_response_exit; } iwpm_user_pid = cb->nlh->nlmsg_pid; + iwpm_user_ulib_version = iwpm_version; + if (iwpm_user_ulib_version < IWPM_UABI_VERSION) + pr_warn_once("%s: Down level iwpmd ABI version. Continuing...", + __func__); atomic_set(&echo_nlmsg_seq, cb->nlh->nlmsg_seq); pr_debug("%s: iWarp Port Mapper (pid = %d) is available!\n", __func__, iwpm_user_pid); @@ -661,7 +693,7 @@ int iwpm_mapping_info_cb(struct sk_buff *skb, struct netlink_callback *cb) iwpm_name = (char *)nla_data(nltb[IWPM_NLA_MAPINFO_ULIB_NAME]); iwpm_version = nla_get_u16(nltb[IWPM_NLA_MAPINFO_ULIB_VER]); if (strcmp(iwpm_ulib_name, iwpm_name) || - iwpm_version != iwpm_ulib_version) { + iwpm_version < IWPM_UABI_VERSION_MIN) { pr_info("%s: Invalid port mapper name = %s version = %d\n", __func__, iwpm_name, iwpm_version); return ret; @@ -754,3 +786,40 @@ int iwpm_mapping_error_cb(struct sk_buff *skb, struct netlink_callback *cb) up(&nlmsg_request->sem); return 0; } + +/* netlink attribute policy for the received hello request */ +static const struct nla_policy hello_policy[IWPM_NLA_HELLO_MAX] = { + [IWPM_NLA_HELLO_ABI_VERSION] = { .type = NLA_U16 } +}; + +/* + * iwpm_hello_cb - Process a port mapper hello request + */ +int iwpm_hello_cb(struct sk_buff *skb, struct netlink_callback *cb) +{ + struct nlattr *nltb[IWPM_NLA_HELLO_MAX]; + const char *msg_type = "Hello request"; + u8 nl_client; + u16 abi_version; + int ret = -EINVAL; + + if (iwpm_parse_nlmsg(cb, IWPM_NLA_HELLO_MAX, hello_policy, nltb, + msg_type)) { + pr_info("%s: Unable to parse nlmsg\n", __func__); + return ret; + } + abi_version = nla_get_u16(nltb[IWPM_NLA_HELLO_ABI_VERSION]); + nl_client = RDMA_NL_GET_CLIENT(cb->nlh->nlmsg_type); + if (!iwpm_valid_client(nl_client)) { + pr_info("%s: Invalid port mapper client = %d\n", + __func__, nl_client); + return ret; + } + iwpm_set_registration(nl_client, IWPM_REG_INCOMPL); + atomic_set(&echo_nlmsg_seq, cb->nlh->nlmsg_seq); + iwpm_user_ulib_version = min_t(u16, IWPM_UABI_VERSION, abi_version); + pr_debug("Using ABI version %u\n", iwpm_user_ulib_version); + iwpm_user_pid = cb->nlh->nlmsg_pid; + ret = iwpm_send_hello(nl_client, iwpm_user_pid, iwpm_user_ulib_version); + return ret; +} diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c index cdb63f3f4de7..98797a4fbe6f 100644 --- a/drivers/infiniband/core/iwpm_util.c +++ b/drivers/infiniband/core/iwpm_util.c @@ -114,7 +114,7 @@ static struct hlist_head *get_mapinfo_hash_bucket(struct sockaddr_storage *, int iwpm_create_mapinfo(struct sockaddr_storage *local_sockaddr, struct sockaddr_storage *mapped_sockaddr, - u8 nl_client) + u8 nl_client, u32 map_flags) { struct hlist_head *hash_bucket_head = NULL; struct iwpm_mapping_info *map_info; @@ -132,6 +132,7 @@ int iwpm_create_mapinfo(struct sockaddr_storage *local_sockaddr, memcpy(&map_info->mapped_sockaddr, mapped_sockaddr, sizeof(struct sockaddr_storage)); map_info->nl_client = nl_client; + map_info->map_flags = map_flags; spin_lock_irqsave(&iwpm_mapinfo_lock, flags); if (iwpm_hash_bucket) { @@ -686,6 +687,14 @@ int iwpm_send_mapinfo(u8 nl_client, int iwpm_pid) if (ret) goto send_mapping_info_unlock; + if (iwpm_user_ulib_version > IWPM_UABI_VERSION_MIN) { + ret = ibnl_put_attr(skb, nlh, sizeof(u32), + &map_info->map_flags, + IWPM_NLA_MAPINFO_FLAGS); + if (ret) + goto send_mapping_info_unlock; + } + nlmsg_end(skb, nlh); iwpm_print_sockaddr(&map_info->local_sockaddr, @@ -754,3 +763,40 @@ int iwpm_mapinfo_available(void) spin_unlock_irqrestore(&iwpm_mapinfo_lock, flags); return full_bucket; } + +int iwpm_send_hello(u8 nl_client, int iwpm_pid, u16 abi_version) +{ + struct sk_buff *skb = NULL; + struct nlmsghdr *nlh; + u32 msg_seq; + const char *err_str = ""; + int ret = -EINVAL; + + skb = iwpm_create_nlmsg(RDMA_NL_IWPM_HELLO, &nlh, nl_client); + if (!skb) { + err_str = "Unable to create a nlmsg"; + goto hello_num_error; + } + nlh->nlmsg_seq = iwpm_get_nlmsg_seq(); + msg_seq = 0; + err_str = "Unable to put attribute of abi_version into nlmsg"; + ret = ibnl_put_attr(skb, nlh, sizeof(u16), &abi_version, + IWPM_NLA_HELLO_ABI_VERSION); + if (ret) + goto hello_num_error; + nlmsg_end(skb, nlh); + + ret = rdma_nl_unicast(skb, iwpm_pid); + if (ret) { + skb = NULL; + err_str = "Unable to send a nlmsg"; + goto hello_num_error; + } + pr_debug("%s: Sent hello abi_version = %u\n", __func__, abi_version); + return 0; +hello_num_error: + pr_info("%s: %s\n", __func__, err_str); + if (skb) + dev_kfree_skb(skb); + return ret; +} diff --git a/drivers/infiniband/core/iwpm_util.h b/drivers/infiniband/core/iwpm_util.h index af1fc14a0d3d..5630df8bfd02 100644 --- a/drivers/infiniband/core/iwpm_util.h +++ b/drivers/infiniband/core/iwpm_util.h @@ -78,6 +78,7 @@ struct iwpm_mapping_info { struct sockaddr_storage local_sockaddr; struct sockaddr_storage mapped_sockaddr; u8 nl_client; + u32 map_flags; }; struct iwpm_remote_info { @@ -266,4 +267,15 @@ int iwpm_parse_nlmsg(struct netlink_callback *cb, int policy_max, * @msg: Message to print */ void iwpm_print_sockaddr(struct sockaddr_storage *sockaddr, char *msg); + +/** + * iwpm_send_hello - Send hello response to iwpmd + * + * @nl_client: The index of the netlink client + * @abi_version: The kernel's abi_version + * + * Returns 0 on success or a negative error code + */ +int iwpm_send_hello(u8 nl_client, int iwpm_pid, u16 abi_version); +extern u16 iwpm_user_ulib_version; #endif diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h index 5cd7701db148..cd727fd6c41e 100644 --- a/include/rdma/iw_cm.h +++ b/include/rdma/iw_cm.h @@ -105,6 +105,18 @@ struct iw_cm_conn_param { u32 qpn; }; +enum iw_flags { + + /* + * This flag allows the iwcm and iwpmd to still advertise + * mappings but the real and mapped port numbers are the + * same. Further, iwpmd will not bind any user socket to + * reserve the port. This is required for soft iwarp + * to play in the port mapped iwarp space. + */ + IW_F_NO_PORT_MAP = BIT(0), +}; + struct iw_cm_verbs { void (*add_ref)(struct ib_qp *qp); @@ -127,6 +139,7 @@ struct iw_cm_verbs { int (*destroy_listen)(struct iw_cm_id *cm_id); char ifname[IFNAMSIZ]; + enum iw_flags driver_flags; }; /** diff --git a/include/rdma/iw_portmap.h b/include/rdma/iw_portmap.h index fda31673a562..84fac196ef80 100644 --- a/include/rdma/iw_portmap.h +++ b/include/rdma/iw_portmap.h @@ -58,6 +58,7 @@ struct iwpm_sa_data { struct sockaddr_storage mapped_loc_addr; struct sockaddr_storage rem_addr; struct sockaddr_storage mapped_rem_addr; + u32 flags; }; /** @@ -205,9 +206,11 @@ int iwpm_get_remote_info(struct sockaddr_storage *mapped_loc_addr, * @local_addr: Local ip/tcp address * @mapped_addr: Mapped local ip/tcp address * @nl_client: The index of the netlink client + * @map_flags: IWPM mapping flags */ int iwpm_create_mapinfo(struct sockaddr_storage *local_addr, - struct sockaddr_storage *mapped_addr, u8 nl_client); + struct sockaddr_storage *mapped_addr, u8 nl_client, + u32 map_flags); /** * iwpm_remove_mapinfo - Remove local and mapped IPv4/IPv6 address @@ -221,4 +224,14 @@ int iwpm_create_mapinfo(struct sockaddr_storage *local_addr, int iwpm_remove_mapinfo(struct sockaddr_storage *local_addr, struct sockaddr_storage *mapped_addr); +/** + * iwpm_hello_cb - Process a hello message from iwpmd + * + * @skb: + * @cb: Contains the received message (payload and netlink header) + * + * Using the received port mapper pid, send the kernel's abi_version + * after adjusting it to support the iwpmd version. + */ +int iwpm_hello_cb(struct sk_buff *skb, struct netlink_callback *cb); #endif /* _IW_PORTMAP_H */ diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h index 42d53e182d5f..0f5263767fb4 100644 --- a/include/uapi/rdma/rdma_netlink.h +++ b/include/uapi/rdma/rdma_netlink.h @@ -35,6 +35,19 @@ enum { RDMA_NL_RDMA_CM_NUM_ATTR, }; +/* The minimum version that the iwpm kernel supports */ +#define IWPM_UABI_VERSION_MIN 3 + +/* The latest version that the iwpm kernel supports */ +#define IWPM_UABI_VERSION 4 + +/* iwarp port mapper message flags */ +enum { + + /* Do not map the port for this IWPM request */ + IWPM_FLAGS_NO_PORT_MAP = (1 << 0), +}; + /* iwarp port mapper op-codes */ enum { RDMA_NL_IWPM_REG_PID = 0, @@ -45,6 +58,7 @@ enum { RDMA_NL_IWPM_HANDLE_ERR, RDMA_NL_IWPM_MAPINFO, RDMA_NL_IWPM_MAPINFO_NUM, + RDMA_NL_IWPM_HELLO, RDMA_NL_IWPM_NUM_OPS }; @@ -83,6 +97,7 @@ enum { IWPM_NLA_MANAGE_MAPPING_UNSPEC = 0, IWPM_NLA_MANAGE_MAPPING_SEQ, IWPM_NLA_MANAGE_ADDR, + IWPM_NLA_MANAGE_FLAGS, IWPM_NLA_MANAGE_MAPPING_MAX }; @@ -98,12 +113,14 @@ enum { }; #define IWPM_NLA_MAPINFO_SEND_MAX 3 +#define IWPM_NLA_REMOVE_MAPPING_MAX 3 enum { IWPM_NLA_QUERY_MAPPING_UNSPEC = 0, IWPM_NLA_QUERY_MAPPING_SEQ, IWPM_NLA_QUERY_LOCAL_ADDR, IWPM_NLA_QUERY_REMOTE_ADDR, + IWPM_NLA_QUERY_FLAGS, IWPM_NLA_QUERY_MAPPING_MAX, }; @@ -129,6 +146,7 @@ enum { IWPM_NLA_MAPINFO_UNSPEC = 0, IWPM_NLA_MAPINFO_LOCAL_ADDR, IWPM_NLA_MAPINFO_MAPPED_ADDR, + IWPM_NLA_MAPINFO_FLAGS, IWPM_NLA_MAPINFO_MAX }; @@ -147,6 +165,12 @@ enum { IWPM_NLA_ERR_MAX }; +enum { + IWPM_NLA_HELLO_UNSPEC = 0, + IWPM_NLA_HELLO_ABI_VERSION, + IWPM_NLA_HELLO_MAX +}; + /* * Local service operations: * RESOLVE - The client requests the local service to resolve a path.
A soft iwarp driver that uses the host TCP stack via a kernel mode socket does not need port mapping. In fact, if the port map daemon, iwpmd, is running, then iwpmd must not try and create/bind a socket to the actual port for a soft iwarp connection, since the driver already has that socket bound. Yet if the soft iwarp driver wants to interoperate with hard iwarp devices that -are- using port mapping, then the soft iwarp driver's mappings still need to be maintained and advertised by the iwpm protocol. This patch enhances the rdma driver<->iwcm interface to allow an iwarp driver to specify that it does not want port mapping. The iwpm kernel<->iwpmd interface is also enhanced to pass up this information on map requests. Care is taken to interoperate with the current iwpmd version (ABI version 3) and only use the new NL attributes if iwpmd supports ABI version 4. The ABI version define has also been created in rdma_netlink.h so both kernel and user code can share it. The iwcm and iwpmd negotiate the ABI version to use with a new HELLO netlink message. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/infiniband/core/iwcm.c | 7 +++- drivers/infiniband/core/iwpm_msg.c | 75 +++++++++++++++++++++++++++++++++++-- drivers/infiniband/core/iwpm_util.c | 48 +++++++++++++++++++++++- drivers/infiniband/core/iwpm_util.h | 12 ++++++ include/rdma/iw_cm.h | 13 +++++++ include/rdma/iw_portmap.h | 15 +++++++- include/uapi/rdma/rdma_netlink.h | 24 ++++++++++++ 7 files changed, 187 insertions(+), 7 deletions(-)