RE: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
> rds_ib_add_one says: > /* Only handle IB (no iWARP) devices */ > but not sure comment is 100% accurate as it checks node type != IB CA. > > rds_ib_laddr_check says: > /* rdma_bind_addr will only succeed for IB & iWARP devices */ > /* due to this, we will claim to support iWARP devices unless we check > node_type. */ > It's similar to above in that it checks node type != IB CA and does not > seem 100% accurate. I noticed these, but I don't know why RDS has this requirement. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
Re: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
On 6/22/2015 6:40 PM, Hefty, Sean wrote: >> Note that there still remain a couple of node type checks in the kernel >> that we may want to remove. There's an IB CA check in cma.c:rdma_notify > > This should check for rdma_cap_ib_cm() IB, RoCE, and OPA ports have the RDMA_CORE_CAP_IB_CM set so this seems OK. For IB switches, there are 2 cases: enhanced switch port 0s can support CM and base switch port 0s would not so this seems better than the IB CA node type current check. > >> as well as in rds/ib.c:rds_ib_add_one and rds_ib_laddr_check and > > Not sure what the underlying reason is for these checks. rds_ib_add_one says: /* Only handle IB (no iWARP) devices */ but not sure comment is 100% accurate as it checks node type != IB CA. rds_ib_laddr_check says: /* rdma_bind_addr will only succeed for IB & iWARP devices */ /* due to this, we will claim to support iWARP devices unless we check node_type. */ It's similar to above in that it checks node type != IB CA and does not seem 100% accurate. Comments and node type check are from Andy Grover back in 2009 (in the original RDS commit). >> an RNIC check in rds/iw.c:rds_iw_add_one and rds_iw_laddr_check > > rdma_cap_iw_cm() iWARP ports are the only ones to have the RDMA_CORE_CAP_IW_CM bit so that seems right. > The intent is to clarify why the checks that exist are made and replace them > with a check that clearly conveys > what the restriction is. So, yes, the use of node_type should be replaced. Understood. -- Hal > - Sean > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
RE: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
> Note that there still remain a couple of node type checks in the kernel > that we may want to remove. There's an IB CA check in cma.c:rdma_notify This should check for rdma_cap_ib_cm() > as well as in rds/ib.c:rds_ib_add_one and rds_ib_laddr_check and Not sure what the underlying reason is for these checks. > an RNIC check in rds/iw.c:rds_iw_add_one and rds_iw_laddr_check rdma_cap_iw_cm() The intent is to clarify why the checks that exist are made and replace them with a check that clearly conveys what the restriction is. So, yes, the use of node_type should be replaced. - Sean -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
Re: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
On 6/22/2015 4:20 PM, Jason Gunthorpe wrote:
> On Thu, Jun 18, 2015 at 07:25:32PM -0400, Hal Rosenstock wrote:
>> Persuant to Liran's comments on node_type on linux-rdma
>> mailing list:
>>
>> In an effort to reform the RDMA core and ULPs to minimize use of
>> node_type in struct ib_device, an additional bit is added to
>> struct ib_device for is_switch (IB switch). This is needed
>> to be initialized by any IB switch device driver. This is a
>> NEW requirement on such device drivers which are all
>> "out of tree".
>>
>> In addition, an ib_switch helper was added to ib_verbs.h
>> based on the is_switch device bit rather than node_type
>> (although those should be consistent).
>>
>> The RDMA core (MAD, SMI, agent, sa_query, multicast, sysfs)
>> as well as (IPoIB and SRP) ULPs are updated where
>> appropriate to use this new helper. In some cases,
>> the helper is now used under the covers of using
>> rdma_[start end]_port rather than the open coding
>> previously used.
>>
>> Signed-off-by: Hal Rosenstock
>
> Looks pretty good now.
>
> Reviewed-By: Jason Gunthorpe
>
> Although a bitfield isn't my preference:
>
>> index 986fddb..b0f898e 100644
>> +++ b/include/rdma/ib_verbs.h
>> @@ -1745,6 +1745,7 @@ struct ib_device {
>> char node_desc[64];
>> __be64 node_guid;
>> u32 local_dma_lkey;
>> +u16 is_switch:1;
>> u8 node_type;
>> u8 phys_port_cnt;
What would be better/what would be your preference ?
Note that there still remain a couple of node type checks in the kernel
that we may want to remove. There's an IB CA check in cma.c:rdma_notify
as well as in rds/ib.c:rds_ib_add_one and rds_ib_laddr_check and an RNIC
check in rds/iw.c:rds_iw_add_one and rds_iw_laddr_check. Should these be
changed not to use node type ?
-- Hal
> Jason
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
Re: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
On Thu, Jun 18, 2015 at 07:25:32PM -0400, Hal Rosenstock wrote:
> Persuant to Liran's comments on node_type on linux-rdma
> mailing list:
>
> In an effort to reform the RDMA core and ULPs to minimize use of
> node_type in struct ib_device, an additional bit is added to
> struct ib_device for is_switch (IB switch). This is needed
> to be initialized by any IB switch device driver. This is a
> NEW requirement on such device drivers which are all
> "out of tree".
>
> In addition, an ib_switch helper was added to ib_verbs.h
> based on the is_switch device bit rather than node_type
> (although those should be consistent).
>
> The RDMA core (MAD, SMI, agent, sa_query, multicast, sysfs)
> as well as (IPoIB and SRP) ULPs are updated where
> appropriate to use this new helper. In some cases,
> the helper is now used under the covers of using
> rdma_[start end]_port rather than the open coding
> previously used.
>
> Signed-off-by: Hal Rosenstock
Reviewed-by: Ira Weiny
Tested-by: Ira Weiny
> ---
> Changes since v1:
> Per Jason and Sean's comments, used Ira's rdma_[start end]_port
> routines where needed and made those routines in ib_verbs.h
> use the new is_switch helper.
>
> diff --git a/drivers/infiniband/core/agent.c b/drivers/infiniband/core/agent.c
> index c7dcfe4..0429040 100644
> --- a/drivers/infiniband/core/agent.c
> +++ b/drivers/infiniband/core/agent.c
> @@ -88,7 +88,7 @@ void agent_send_response(const struct ib_mad_hdr *mad_hdr,
> const struct ib_grh *
> struct ib_ah *ah;
> struct ib_mad_send_wr_private *mad_send_wr;
>
> - if (device->node_type == RDMA_NODE_IB_SWITCH)
> + if (rdma_cap_ib_switch(device))
> port_priv = ib_get_agent_port(device, 0);
> else
> port_priv = ib_get_agent_port(device, port_num);
> @@ -122,7 +122,7 @@ void agent_send_response(const struct ib_mad_hdr
> *mad_hdr, const struct ib_grh *
> memcpy(send_buf->mad, mad_hdr, resp_mad_len);
> send_buf->ah = ah;
>
> - if (device->node_type == RDMA_NODE_IB_SWITCH) {
> + if (rdma_cap_ib_switch(device)) {
> mad_send_wr = container_of(send_buf,
> struct ib_mad_send_wr_private,
> send_buf);
> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
> index a4b1466..c82d751 100644
> --- a/drivers/infiniband/core/mad.c
> +++ b/drivers/infiniband/core/mad.c
> @@ -769,7 +769,7 @@ static int handle_outgoing_dr_smp(struct
> ib_mad_agent_private *mad_agent_priv,
> bool opa = rdma_cap_opa_mad(mad_agent_priv->qp_info->port_priv->device,
>
> mad_agent_priv->qp_info->port_priv->port_num);
>
> - if (device->node_type == RDMA_NODE_IB_SWITCH &&
> + if (rdma_cap_ib_switch(device) &&
> smp->mgmt_class == IB_MGMT_CLASS_SUBN_DIRECTED_ROUTE)
> port_num = send_wr->wr.ud.port_num;
> else
> @@ -787,7 +787,8 @@ static int handle_outgoing_dr_smp(struct
> ib_mad_agent_private *mad_agent_priv,
> if ((opa_get_smp_direction(opa_smp)
>? opa_smp->route.dr.dr_dlid : opa_smp->route.dr.dr_slid) ==
>OPA_LID_PERMISSIVE &&
> - opa_smi_handle_dr_smp_send(opa_smp, device->node_type,
> + opa_smi_handle_dr_smp_send(opa_smp,
> + rdma_cap_ib_switch(device),
> port_num) == IB_SMI_DISCARD) {
> ret = -EINVAL;
> dev_err(&device->dev, "OPA Invalid directed route\n");
> @@ -810,7 +811,7 @@ static int handle_outgoing_dr_smp(struct
> ib_mad_agent_private *mad_agent_priv,
> } else {
> if ((ib_get_smp_direction(smp) ? smp->dr_dlid : smp->dr_slid) ==
>IB_LID_PERMISSIVE &&
> - smi_handle_dr_smp_send(smp, device->node_type, port_num) ==
> + smi_handle_dr_smp_send(smp, rdma_cap_ib_switch(device),
> port_num) ==
>IB_SMI_DISCARD) {
> ret = -EINVAL;
> dev_err(&device->dev, "Invalid directed route\n");
> @@ -2030,7 +2031,7 @@ static enum smi_action handle_ib_smi(const struct
> ib_mad_port_private *port_priv
> struct ib_smp *smp = (struct ib_smp *)recv->mad;
>
> if (smi_handle_dr_smp_recv(smp,
> -port_priv->device->node_type,
> +rdma_cap_ib_switch(port_priv->device),
> port_num,
> port_priv->device->phys_port_cnt) ==
> IB_SMI_DISCARD)
> @@ -2042,13 +2043,13 @@ static enum smi_action handle_ib_smi(const struct
> ib_mad_port_private *port_priv
>
> if (retsmi == IB_SMI_SEND) { /* don't forward */
> if (smi_handle_dr_smp_send(smp,
> -po
Re: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
On Thu, Jun 18, 2015 at 07:25:32PM -0400, Hal Rosenstock wrote:
> Persuant to Liran's comments on node_type on linux-rdma
> mailing list:
>
> In an effort to reform the RDMA core and ULPs to minimize use of
> node_type in struct ib_device, an additional bit is added to
> struct ib_device for is_switch (IB switch). This is needed
> to be initialized by any IB switch device driver. This is a
> NEW requirement on such device drivers which are all
> "out of tree".
>
> In addition, an ib_switch helper was added to ib_verbs.h
> based on the is_switch device bit rather than node_type
> (although those should be consistent).
>
> The RDMA core (MAD, SMI, agent, sa_query, multicast, sysfs)
> as well as (IPoIB and SRP) ULPs are updated where
> appropriate to use this new helper. In some cases,
> the helper is now used under the covers of using
> rdma_[start end]_port rather than the open coding
> previously used.
>
> Signed-off-by: Hal Rosenstock
Looks pretty good now.
Reviewed-By: Jason Gunthorpe
Although a bitfield isn't my preference:
> index 986fddb..b0f898e 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -1745,6 +1745,7 @@ struct ib_device {
> char node_desc[64];
> __be64 node_guid;
> u32 local_dma_lkey;
> + u16 is_switch:1;
> u8 node_type;
> u8 phys_port_cnt;
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
RE: [PATCHv2 RFC] IB: Add rdma_cap_ib_switch helper and use where appropriate
> In an effort to reform the RDMA core and ULPs to minimize use of > node_type in struct ib_device, an additional bit is added to > struct ib_device for is_switch (IB switch). This is needed > to be initialized by any IB switch device driver. This is a > NEW requirement on such device drivers which are all > "out of tree". > > In addition, an ib_switch helper was added to ib_verbs.h > based on the is_switch device bit rather than node_type > (although those should be consistent). > > The RDMA core (MAD, SMI, agent, sa_query, multicast, sysfs) > as well as (IPoIB and SRP) ULPs are updated where > appropriate to use this new helper. In some cases, > the helper is now used under the covers of using > rdma_[start end]_port rather than the open coding > previously used. > > Signed-off-by: Hal Rosenstock Assuming that the final version matches this one: Reviewed-by: Sean Hefty -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
