Re: [PATCH v2 for-next 5/7] IB/mlx4: Add IB counters table

[Matan Barak]

On Fri, Dec 25, 2015 at 5:56 PM, Hal Rosenstock  wrote:
> On 12/25/2015 9:50 AM, Hal Rosenstock wrote:
>> On 12/24/2015 11:09 AM, Matan Barak wrote:
>>> On Thu, Dec 24, 2015 at 4:07 PM, Matan Barak  
>>> wrote:
 On Thu, Dec 24, 2015 at 2:38 PM, Or Gerlitz  wrote:
> On 12/24/2015 12:42 PM, Sagi Grimberg wrote:
>>
>>
 This patch seems to generate a list corruption [1] when I test
 with Doug's for-4.5 tree. Eran, care to take a look at this?
>>>
>>>
>>> This patch is part from a series that was introduced in 4.3-rc1 [1],
>>
>>
>> Then something else broke it. Can people check their patches on doug's
>> tree? At the moment it's unusable...
>

 Leon and I have checked Doug's tree with mlx4_ib disabled and we
 didn't encounter any error.
 We ran ucmatose over IB connection (in mlx5) and it worked flawlessly.

>
> Yes, I checked the branch up to commit 882f3b3 "Merge branches
> '4.5/Or-cleanup' and '4.5/rdma-cq' into k.o/for-4.5" and it works (rping,
> ibv_rc_pingpong over top of mlx4 VPI)
>
>>>
>>> Regarding mlx4, Eran and I analyzed it. We didn't test that, but it
>>> seems like the bug is introduced in the 64bit counters test. Here's a
>>> proposal:
>>>
>>> diff --git a/drivers/infiniband/core/sysfs.c 
>>> b/drivers/infiniband/core/sysfs.c
>>> index 539040f..8da3c83 100644
>>> --- a/drivers/infiniband/core/sysfs.c
>>> +++ b/drivers/infiniband/core/sysfs.c
>>> @@ -714,11 +714,12 @@ err:
>>>   * Figure out which counter table to use depending on
>>>   * the device capabilities.
>>>   */
>>> -static struct attribute_group *get_counter_table(struct ib_device *dev)
>>> +static struct attribute_group *get_counter_table(struct ib_device *dev,
>>> +  int port_num)
>>>  {
>>> struct ib_class_port_info cpi;
>>>
>>> -   if (get_perf_mad(dev, 0, IB_PMA_CLASS_PORT_INFO,
>>> + if (get_perf_mad(dev, port_num, IB_PMA_CLASS_PORT_INFO,
>>> , 40, sizeof(cpi)) >= 0) {
>>
>> Your proposal is similar to earlier version of Christoph's patch but was
>> changed since ClassPortInfo attribute does not have PortSelect field
>> like other PerfMgt attributes which is where this port num would be
>> placed. In ClassPortInfo attribute, that location would be the
>> ClassVersion field that would be set to port number in PerfMgt Get query.
>
> In actuality, I don't think it really matters as this is a Get not a Set
> and the PMA would do the right thing even if some field in the CPI were
> stepped on.
>

Well, it does matter as it calls the vendor driver with port_num = 0.
Since the kernel is trusted, the vendor driver expects a valid port number.
Giving it an invalid number might result in memory corruptions, as
demonstrated in this case.

>> -- Hal

Matan

>>
>>>
>>> if (cpi.capability_mask && IB_PMA_CLASS_CAP_EXT_WIDTH)
>>> @@ -776,7 +777,7 @@ static int add_port(struct ib_device *device, int 
>>> port_num,
>>> goto err_put;
>>> }
>>>
>>> -   p->pma_table = get_counter_table(device);
>>> + p->pma_table = get_counter_table(device, port_num);
>>> ret = sysfs_create_group(>kobj, p->pma_table);
>>> if (ret)
>>> goto err_put_gid_attrs;
>>>
>>>
> --
> 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
>>> --
>>> 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
>>>
--
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

Re: [RFC] Generic InfiniBand transport done in software

[Moni Shoua]

>
> Point is others have looked at the code. No issues have been called out to
> date as to why what is there won't work for everyone.
>
http://marc.info/?l=linux-rdma=144968107508268=2
Your answer to the send() issue is first an agreement with the comment
and later says that it can't be done because of how PIO and SDMA
(Intel specific implementation)
This is an example for a discussion that never ended with an agreement.


>
> Yes it is specific to Intel *now*, that doesn't mean it should stay that
> way. Rdmavt could, and in my opinion should, be extended to support
> soft-roce. I don't think replicating the same thing is a great idea.
>
But you post *now* a so called generic driver so it must now fit any
possible driver (including Soft RoCE)
> As to the location, where do you think it should go. drivers/infiniband/sw
> makes the most sense to me, but open to suggestions.
>
> And for the question of why publish when it's not ready, the better question
> is why not?  Is it not good to see the work in progress as it evolves so the
> community can provide feedback?
>
What kind of a feedback you expect when I don't have an idea about
your plans for rdmavt
Interfaces, flows, data structures... all is missing from the
documentation to rdmavt.
--
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

[PATCH 0/2] InfiniBand-iSER: Refactoring for two function implementations

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 27 Dec 2015 13:12:10 +0100
Subject: [PATCH 0/2] InfiniBand-iSER: Refactoring for two function 
implementations

I suggest to return directly instead of using the jump label "err"
in two functions (which are working without clean-up there).

Markus Elfring (2):
  One jump label less in iser_reg_sig_mr()
  One jump label less in isert_reg_sig_mr()

 drivers/infiniband/ulp/iser/iser_memory.c | 5 ++---
 drivers/infiniband/ulp/isert/ib_isert.c   | 7 +++
 2 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.6.3

--
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

[PATCH 1/2] InfiniBand-iSER: One jump label less in iser_reg_sig_mr()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 27 Dec 2015 11:41:42 +0100

This issue was detected by using the Coccinelle software.

1. Let us return directly if a call of the iser_set_sig_attrs()
   function failed.

2. Delete the jump label "err" then.

3. Return zero as a constant at the end.

Signed-off-by: Markus Elfring 
---
 drivers/infiniband/ulp/iser/iser_memory.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_memory.c 
b/drivers/infiniband/ulp/iser/iser_memory.c
index ea765fb..14e08b3 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -443,7 +443,7 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
memset(sig_attrs, 0, sizeof(*sig_attrs));
ret = iser_set_sig_attrs(iser_task->sc, sig_attrs);
if (ret)
-   goto err;
+   return ret;
 
iser_set_prot_checks(iser_task->sc, _attrs->check_mask);
 
@@ -475,8 +475,7 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
iser_dbg("lkey=0x%x rkey=0x%x addr=0x%llx length=%u\n",
 sig_reg->sge.lkey, sig_reg->rkey, sig_reg->sge.addr,
 sig_reg->sge.length);
-err:
-   return ret;
+   return 0;
 }
 
 static int iser_fast_reg_mr(struct iscsi_iser_task *iser_task,
-- 
2.6.3

--
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

[PATCH 2/2] InfiniBand-iSER-target: One jump label less in isert_reg_sig_mr()

[SF Markus Elfring]

From: Markus Elfring 
Date: Sun, 27 Dec 2015 12:54:52 +0100

This issue was detected by using the Coccinelle software.

1. Let us return directly if a call of the function "isert_set_sig_attrs"
   or "ib_post_send" failed.

2. Delete the jump label "err" then.

3. Return zero as a constant at the end.

Signed-off-by: Markus Elfring 
---
 drivers/infiniband/ulp/isert/ib_isert.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c 
b/drivers/infiniband/ulp/isert/ib_isert.c
index 8a51c3b..9b22db0 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -2660,7 +2660,7 @@ isert_reg_sig_mr(struct isert_conn *isert_conn,
memset(_attrs, 0, sizeof(sig_attrs));
ret = isert_set_sig_attrs(se_cmd, _attrs);
if (ret)
-   goto err;
+   return ret;
 
sig_attrs.check_mask = isert_set_prot_checks(se_cmd->prot_checks);
 
@@ -2688,7 +2688,7 @@ isert_reg_sig_mr(struct isert_conn *isert_conn,
ret = ib_post_send(isert_conn->qp, wr, _wr);
if (ret) {
isert_err("fast registration failed, ret:%d\n", ret);
-   goto err;
+   return ret;
}
fr_desc->ind &= ~ISERT_SIG_KEY_VALID;
 
@@ -2706,8 +2706,7 @@ isert_reg_sig_mr(struct isert_conn *isert_conn,
isert_dbg("sig_sge: addr: 0x%llx  length: %u lkey: %x\n",
  rdma_wr->ib_sg[SIG].addr, rdma_wr->ib_sg[SIG].length,
  rdma_wr->ib_sg[SIG].lkey);
-err:
-   return ret;
+   return 0;
 }
 
 static int
-- 
2.6.3

--
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

Re: [PATCH 0/2] InfiniBand-iSER: Refactoring for two function implementations

[Leon Romanovsky]

On Sun, Dec 27, 2015 at 01:36:30PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sun, 27 Dec 2015 13:12:10 +0100
> Subject: [PATCH 0/2] InfiniBand-iSER: Refactoring for two function 
> implementations
Just a note for the future submissions (no need to respin), we are using
different subject line: InfiniBand-iSER: --> IB/iser:

> 
> I suggest to return directly instead of using the jump label "err"
> in two functions (which are working without clean-up there).
> 
> Markus Elfring (2):
>   One jump label less in iser_reg_sig_mr()
>   One jump label less in isert_reg_sig_mr()
> 
>  drivers/infiniband/ulp/iser/iser_memory.c | 5 ++---
>  drivers/infiniband/ulp/isert/ib_isert.c   | 7 +++
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> -- 
> 2.6.3
> 
> --
> 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
--
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

Re: [PATCH 0/2] InfiniBand-iSER: Refactoring for two function implementations

[Leon Romanovsky]

On Sun, Dec 27, 2015 at 01:36:30PM +0100, SF Markus Elfring wrote:
> From: Markus Elfring 
> Date: Sun, 27 Dec 2015 13:12:10 +0100
> Subject: [PATCH 0/2] InfiniBand-iSER: Refactoring for two function 
> implementations
> 
> I suggest to return directly instead of using the jump label "err"
> in two functions (which are working without clean-up there).
> 
> Markus Elfring (2):
>   One jump label less in iser_reg_sig_mr()
>   One jump label less in isert_reg_sig_mr()
Looks good,
Reviewed-by: Leon Romanovsky 

> 
>  drivers/infiniband/ulp/iser/iser_memory.c | 5 ++---
>  drivers/infiniband/ulp/isert/ib_isert.c   | 7 +++
>  2 files changed, 5 insertions(+), 7 deletions(-)
> 
> -- 
> 2.6.3
> 
> --
> 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
--
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