Re: [PATCH for-next] IB/usnic: Expose flows via debugfs
On 08/01/2014 20:57, Upinder Malhi (umalhi) wrote: Or, Yeah, I did think about extending the existing infrastructure to export HW specific stats and exposing some stats via standard infrastructure. Besides the below, there are few other drawbacks with exposing statistics via netlink: 1) Having a two part solution, users space and kernel, will make changing the code more difficult. Anytime another attributed is exposed, code in the kernel needs to be added to handle backwards compatibility with userspace (as I said we are going to add more stuff incrementally). There are thousands if not millions LOCs over the kernel and user space tools which use netlink. Indeed when you have two for tango you sometimes change one side, sometimes the other and sometimes both. A claim that its easier to maintain things when all the code resides in the kernel can't be really taken seriously into account. Netlink is used all over the place, so everyone's wrong? 2) The Cisco VIC series cards, that is our NIC, cannot do flow stats well. Specially, it only reports Rx byte count for a flow and doesn't report any statistics on the Tx side. Hence, exposing these via a standard interface to a user is going to be confusing and misleading. 1st use the standard/existing interface to report the open sessions and later we'll take it from there re the byte counts. Hence, at least for Cisco VIC, we want to keep these flow stats in debugfs where they can be easily extended and extra effort is required to get to them. Upinder On Jan 8, 2014, at 1:13 AM, Or Gerlitz ogerl...@mellanox.com wrote: On 08/01/2014 00:29, Upinder Malhi (umalhi) wrote: Or, The flows contain Cisco VIC specific stuff - Ex. the hardware flow id; and they will contain more cisco specific things. Hence, they are exported via debugfs. You should be able to enhance the rdma netlink infrastructure to allow for exporting HW dependent attributes to user space -- did you look on that? Also, you should make sure to expose the non HW specific attributes of the sessions through the standard infrastructure. Or. Upinder On Dec 22, 2013, at 2:23 AM, Or Gerlitz ogerl...@mellanox.com wrote: On 20/12/2013 23:37, Upinder Malhi (umalhi) wrote: This patch depends onhttp://www.spinics.net/lists/linux-rdma/msg18193.html Why use proprietary debugfs code to display flows? you can (and should) use the rdma subsystem netlink infrastructure for that, see these two commits 753f618 RDMA/cma: Add support for netlink statistics export b2cbae2 RDMA: Add netlink infrastructure -- 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
[PATCH] amso1100: Add check if cache memory was allocated before freeing it
From: Svetlana Mavrina another.kar...@gmail.com There is a path in handle_vq() where kmem_cache_free() can be called with pointer to a local variable. It can happen if vq_repbuf_alloc() failed to allocate memory from cache and req is NULL. The patch adds check if cache memory was allocated before freeing it. Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Svetlana Mavrina another.kar...@gmail.com Reviewed-by: Alexey Khoroshilov khoroshi...@ispras.ru --- drivers/infiniband/hw/amso1100/c2_intr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/infiniband/hw/amso1100/c2_intr.c b/drivers/infiniband/hw/amso1100/c2_intr.c index 8951db4ae29d..3a17d9b36dba 100644 --- a/drivers/infiniband/hw/amso1100/c2_intr.c +++ b/drivers/infiniband/hw/amso1100/c2_intr.c @@ -169,7 +169,8 @@ static void handle_vq(struct c2_dev *c2dev, u32 mq_index) * We should never get here, as the adapter should * never send us a reply that we're not expecting. */ - vq_repbuf_free(c2dev, host_msg); + if (reply_msg != NULL) + vq_repbuf_free(c2dev, host_msg); pr_debug(handle_vq: UNEXPECTEDLY got NULL req\n); return; } -- 1.8.1.2 -- 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 09/11] IB/isert: Accept RDMA_WRITE completions
On 1/11/2014 11:14 PM, Or Gerlitz wrote: On Thu, Jan 9, 2014 at 6:40 PM, Sagi Grimberg sa...@mellanox.com wrote: In case of protected transactions, we will need to check the protection status of the transaction before sending SCSI response. So be ready for RDMA_WRITE completions. currently we don't ask for these completions, but for T10-PI we will. @@ -1721,9 +1735,9 @@ __isert_send_completion(struct iser_tx_desc *tx_desc, isert_conn, ib_dev); break; case ISER_IB_RDMA_WRITE: - pr_err(isert_send_completion: Got ISER_IB_RDMA_WRITE\n); - dump_stack(); - break; + pr_debug(isert_send_completion: Got ISER_IB_RDMA_WRITE\n); + atomic_dec(isert_conn-post_send_buf_count); + isert_completion_rdma_write(tx_desc, isert_cmd); are we doing fall through here? why? O, somehow i missed it in squash... Will fix, Thanks! case ISER_IB_RDMA_READ: pr_debug(isert_send_completion: Got ISER_IB_RDMA_READ:\n); -- 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 06/11] IB/isert: Initialize T10-PI resources
On 1/11/2014 11:09 PM, Or Gerlitz wrote:
On Thu, Jan 9, 2014 at 6:40 PM, Sagi Grimberg sa...@mellanox.com wrote:
@@ -557,8 +629,14 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct
rdma_cm_event *event)
goto out_mr;
}
+ if (pi_support !device-pi_capable) {
+ pr_err(Protection information requested but not supported\n);
+ ret = -EINVAL;
+ goto out_mr;
+ }
+
if (device-use_fastreg) {
- ret = isert_conn_create_fastreg_pool(isert_conn);
+ ret = isert_conn_create_fastreg_pool(isert_conn, pi_support);
just a nit, the pi_support bit can be looked up from the isert_conn
struct, isn't it?
if (ret) {
pr_err(Conn: %p failed to create fastreg pool\n,
isert_conn);
@@ -566,7 +644,7 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct
rdma_cm_event *event)
}
}
- ret = isert_conn_setup_qp(isert_conn, cma_id);
+ ret = isert_conn_setup_qp(isert_conn, cma_id, pi_support);
if (ret)
goto out_conn_dev;
@@ -2193,7 +2271,7 @@ isert_fast_reg_mr(struct fast_reg_descriptor *fr_desc,
pagelist_len = isert_map_fr_pagelist(ib_dev, sg_start, sg_nents,
fr_desc-data_frpl-page_list[0]);
- if (!fr_desc-valid) {
+ if (!fr_desc-data_key_valid) {
memset(inv_wr, 0, sizeof(inv_wr));
inv_wr.opcode = IB_WR_LOCAL_INV;
inv_wr.ex.invalidate_rkey = fr_desc-data_mr-rkey;
@@ -2225,7 +2303,7 @@ isert_fast_reg_mr(struct fast_reg_descriptor *fr_desc,
pr_err(fast registration failed, ret:%d\n, ret);
return ret;
}
- fr_desc-valid = false;
+ fr_desc-data_key_valid = false;
ib_sge-lkey = fr_desc-data_mr-lkey;
ib_sge-addr = fr_desc-data_frpl-page_list[0] + page_off;
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h
b/drivers/infiniband/ulp/isert/ib_isert.h
index 708a069..fab8b50 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -48,11 +48,21 @@ struct iser_tx_desc {
struct ib_send_wr send_wr;
} __packed;
+struct pi_context {
+ struct ib_mr *prot_mr;
+ boolprot_key_valid;
+ struct ib_fast_reg_page_list *prot_frpl;
+ struct ib_mr *sig_mr;
+ boolsig_key_valid;
+};
+
struct fast_reg_descriptor {
- struct list_headlist;
- struct ib_mr*data_mr;
- struct ib_fast_reg_page_list*data_frpl;
- boolvalid;
+ struct list_headlist;
+ struct ib_mr *data_mr;
+ booldata_key_valid;
+ struct ib_fast_reg_page_list *data_frpl;
+ boolprotected;
no need for many bools in one structure... each one needs a bit,
correct? so embed them in one variable
I figured it will be more explicit this way.
protected boolean indicates if we should check the data-integrity
status, and the other 3 indicates if the relevant MR is valid (no need
to execute local invalidation).
Do you think I should compact it somehow? usually xxx_valid booleans
will align together although not always.
+ struct pi_context *pi_ctx;
};
struct isert_rdma_wr {
@@ -140,6 +150,7 @@ struct isert_cq_desc {
struct isert_device {
int use_fastreg;
+ boolpi_capable;
this one (and its such) is/are derived from the ib device
capabilities, so I would suggest to keep a copy of the caps instead of
derived bools
Yes, I'll keep the device capabilities instead.
int cqs_used;
int refcount;
int cq_active_qps[ISERT_MAX_CQ];
--
To unsubscribe from this list: send the line unsubscribe target-devel 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 06/11] IB/isert: Initialize T10-PI resources
On 12/01/2014 14:41, Sagi Grimberg wrote:
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -48,11 +48,21 @@ struct iser_tx_desc {
struct ib_send_wr send_wr;
} __packed;
+struct pi_context {
+ struct ib_mr *prot_mr;
+ bool prot_key_valid;
+ struct ib_fast_reg_page_list *prot_frpl;
+ struct ib_mr *sig_mr;
+ bool sig_key_valid;
+};
+
struct fast_reg_descriptor {
- struct list_headlist;
- struct ib_mr*data_mr;
- struct ib_fast_reg_page_list*data_frpl;
- boolvalid;
+ struct list_headlist;
+ struct ib_mr *data_mr;
+ bool data_key_valid;
+ struct ib_fast_reg_page_list *data_frpl;
+ boolprotected;
no need for many bools in one structure... each one needs a bit,
correct? so embed them in one variable
I figured it will be more explicit this way. protected boolean
indicates if we should check the data-integrity status, and the other
3 indicates if the relevant MR is valid (no need to execute local
invalidation). Do you think I should compact it somehow? usually
xxx_valid booleans will align together although not always.
I didn't note there are so many booleans there... sure, my personal
preference is compaction, e.g have one field names flags and multiple
bit locations marking the different flags e.g
FAST_REG_DESC_VALID
FAST_REG_DESC_DATA_KEY_VALID
FAST_REG_DESC_PROTECTED
etc
--
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 infiniband-diags] libibnetdisc/ibnetdisc.c: fix insert of invalid lid 0xFFFF into lid_port hash_table
Good catch some comments below.
-Original Message-
From: Hal Rosenstock [mailto:h...@dev.mellanox.co.il]
Sent: Monday, December 30, 2013 6:51 AM
To: Ira Weiny
Cc: linux-rdma (linux-rdma@vger.kernel.org); Dan Ben-Yosef
Subject: [PATCH infiniband-diags] libibnetdisc/ibnetdisc.c: fix insert of
invalid
lid 0x into lid_port hash_table
From: Dan Ben Yosef da...@mellanox.com
Signed-off-by: Dan Ben Yosef da...@mellanox.com
---
libibnetdisc/src/ibnetdisc.c | 12 +++-
1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/libibnetdisc/src/ibnetdisc.c b/libibnetdisc/src/ibnetdisc.c index
9d120dd..121fe35 100644
--- a/libibnetdisc/src/ibnetdisc.c
+++ b/libibnetdisc/src/ibnetdisc.c
@@ -647,11 +647,13 @@ void add_to_portlid_hash(ibnd_port_t * port,
GHashTable *htable)
uint16_t base_lid = port-base_lid;
uint16_t lid_mask = ((1 port-lmc) -1);
uint16_t lid = 0;
-
- /* We add the port for all lids
- * so it is easier to find any random lid specified */
- for (lid = base_lid; lid = (base_lid + lid_mask); lid++) {
- g_hash_table_insert(htable, GINT_TO_POINTER(lid), port);
+ /* 0 valid lid = 0xbfff */
+ if (base_lid 0 base_lid = 0xbfff) {
Shouldn't we check the max LID (based on LMC) here to make sure they are all
vaild?
Ira
+ /* We add the port for all lids
+ * so it is easier to find any random lid specified */
+ for (lid = base_lid; lid = (base_lid + lid_mask); lid++) {
+ g_hash_table_insert(htable, GINT_TO_POINTER(lid),
port);
+ }
}
}
--
1.7.8.2
--
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 infiniband-diags] libibnetdisc/ibnetdisc.c: improve log information to stdout
-Original Message-
From: Dan Ben Yosef [mailto:da...@mellanox.com]
Sent: Wednesday, January 01, 2014 10:44 PM
Subject: [PATCH infiniband-diags] libibnetdisc/ibnetdisc.c: improve log
information to stdout
Write to stdout the route-path when smp returns with status error.
As part of a library this will affect many tools outputs and perhaps
subsequently any scripts written against those tools.
Is this really a requirement or simply a nice to have?
Ira
Signed-off-by: Dan Ben Yosef da...@mellanox.com
---
libibnetdisc/src/internal.h | 10 +++---
libibnetdisc/src/query_smp.c |6 ++
2 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/libibnetdisc/src/internal.h b/libibnetdisc/src/internal.h index
1ccd29c..47b2a4c 100644
--- a/libibnetdisc/src/internal.h
+++ b/libibnetdisc/src/internal.h
@@ -42,12 +42,16 @@
#include complib/cl_qmap.h
#include glib.h
+#define IBND_ERROR(fmt, ...) \
+ fprintf(stderr, %s:%u; fmt, __FILE__, __LINE__, ##
__VA_ARGS__)
+
+#define IBND_INFO(fmt, ...) \
+ printf(%s:%u; fmt, __FILE__, __LINE__, ##
__VA_ARGS__);
+
#define IBND_DEBUG(fmt, ...) \
if (ibdebug) { \
- printf(%s:%u; fmt, __FILE__, __LINE__, ##
__VA_ARGS__); \
+ IBND_INFO(fmt,## __VA_ARGS__); \
}
-#define IBND_ERROR(fmt, ...) \
- fprintf(stderr, %s:%u; fmt, __FILE__, __LINE__, ##
__VA_ARGS__)
/* HASH table defines */
#define HASHGUID(guid) ((uint32_t)(((uint32_t)(guid) * 101) ^
((uint32_t)((guid) 32) * 103))) diff --git a/libibnetdisc/src/query_smp.c
b/libibnetdisc/src/query_smp.c index 28620b4..ac4984e 100644
--- a/libibnetdisc/src/query_smp.c
+++ b/libibnetdisc/src/query_smp.c
@@ -192,6 +192,9 @@ static int process_one_recv(smp_engine_t * engine)
goto error;
if ((status = umad_status(umad))) {
+ IBND_INFO(umad (%s Attr 0x%x:%u) bad status %d; %s\n,
+portid2str(smp-path), smp-rpc.attr.id,
+smp-rpc.attr.mod, status, strerror(status));
IBND_ERROR(umad (%s Attr 0x%x:%u) bad status %d;
%s\n,
portid2str(smp-path), smp-rpc.attr.id,
smp-rpc.attr.mod, status, strerror(status)); @@ -
199,6 +202,9 @@ static int process_one_recv(smp_engine_t * engine)
rc = mlnx_ext_port_info_err(engine, smp, mad,
smp-cb_data);
} else if ((status = mad_get_field(mad, 0, IB_DRSMP_STATUS_F))) {
+ IBND_INFO(mad (%s Attr 0x%x:%u) bad status 0x%x\n,
+portid2str(smp-path), smp-rpc.attr.id,
+smp-rpc.attr.mod, status);
IBND_ERROR(mad (%s Attr 0x%x:%u) bad status 0x%x\n,
portid2str(smp-path), smp-rpc.attr.id,
smp-rpc.attr.mod, status);
--
1.7.1
--
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
