[PATCH rdma-next V2 0/5] Trivial fixes for 4.7

[PATCH rdma-next V2 0/5] Trivial fixes for 4.7

[Leon Romanovsky]

Hi Doug,

I'm sending to you small collection of trivial fixes to core code.

It fixes forgotten skb release call, potential overrun in CMA/SA, integration of
ib_addr.ko into ib_core.ko and one code simplifications which doesn't introduce
any code change behavior.

Available in the "topic/fix-core" topic branch of this git repo:

git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git

Or for browsing:

https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/fix-core

Changes since v2:
 * Removed iwcm_nl_cb_table size initialization

Changes since v1:
 * Added Reviewed-by tags
 * Revisited titles
 * Removed RDMA_NL_RDMA_CM_NUM_OPS and RDMA_NL_LS_NUM_OPS
 * Removed useless enumeration value
 * Converted ib_addr module to be integral part of ib_core module

Leon Romanovsky (1):
  IB/core: Integrate IB address resolution module into core

Mark Bloch (4):
  IB/IWPM: Fix a potential skb leak
  IB/core: Remove unnecessary check in ibnl_rcv_msg
  IB/core: Fix a potential array overrun in CMA and SA agent
  IB/SA: Use correct free function

 drivers/infiniband/core/Makefile    |  6 ++----
 drivers/infiniband/core/addr.c      | 11 ++---------
 drivers/infiniband/core/cma.c       |  3 ++-
 drivers/infiniband/core/device.c    | 11 ++++++++++-
 drivers/infiniband/core/iwcm.c      |  2 +-
 drivers/infiniband/core/iwpm_util.c |  1 +
 drivers/infiniband/core/netlink.c   |  5 ++---
 drivers/infiniband/core/sa_query.c  |  4 ++--
 include/rdma/ib_addr.h              |  3 +++
 9 files changed, 25 insertions(+), 21 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V2 1/5] IB/IWPM: Fix a potential skb leak

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

In case ibnl_put_msg fails in send_nlmsg_done,
the function returns with -ENOMEM without freeing.

This patch fixes this behavior.

Fixes: 30dc5e63d6a5 ("RDMA/core: Add support for iWARP Port Mapper user space service")
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/core/iwpm_util.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/core/iwpm_util.c b/drivers/infiniband/core/iwpm_util.c
index 9b2bf2f..b65e06c 100644
--- a/drivers/infiniband/core/iwpm_util.c
+++ b/drivers/infiniband/core/iwpm_util.c
@@ -634,6 +634,7 @@ static int send_nlmsg_done(struct sk_buff *skb, u8 nl_client, int iwpm_pid)
 	if (!(ibnl_put_msg(skb, &nlh, 0, 0, nl_client,
 			   RDMA_NL_IWPM_MAPINFO, NLM_F_MULTI))) {
 		pr_warn("%s Unable to put NLMSG_DONE\n", __func__);
+		dev_kfree_skb(skb);
 		return -ENOMEM;
 	}
 	nlh->nlmsg_type = NLMSG_DONE;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V2 2/5] IB/core: Remove unnecessary check in ibnl_rcv_msg

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

RDMA_NL_GET_OP is defined like this: (type & ((1 << 10) - 1))
which means op (defined as an int) can never be a negative number.

Fixes: b2cbae2c2487 ('RDMA: Add netlink infrastructure')
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/netlink.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index d47df93..9b8c20c 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -151,12 +151,11 @@ static int ibnl_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct ibnl_client *client;
 	int type = nlh->nlmsg_type;
 	int index = RDMA_NL_GET_CLIENT(type);
-	int op = RDMA_NL_GET_OP(type);
+	unsigned int op = RDMA_NL_GET_OP(type);
 
 	list_for_each_entry(client, &client_list, list) {
 		if (client->index == index) {
-			if (op < 0 || op >= client->nops ||
-			    !client->cb_table[op].dump)
+			if (op >= client->nops || !client->cb_table[op].dump)
 				return -EINVAL;
 
 			/*
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V2 3/5] IB/core: Fix a potential array overrun in CMA and SA agent

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Fix array overrun when going over callback table.
In declaration of callback table, the max size isn't provided and
in registration phase, it is provided.

There is potential scenario where a new operation is added
and it is not supported by current client. The acceptance of
such operation by ib_netlink will cause to array overrun.

Fixes: 809d5fc9bf65 ("infiniband: pass rdma_cm module to netlink_dump_start")
Fixes: b493d91d333e ("iwcm: common code for port mapper")
Fixes: 2ca546b92a02 ("IB/sa: Route SA pathrecord query through netlink")
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/cma.c      | 3 ++-
 drivers/infiniband/core/iwcm.c     | 2 +-
 drivers/infiniband/core/sa_query.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 93ab0ae..b575bd5 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -4294,7 +4294,8 @@ static int __init cma_init(void)
 	if (ret)
 		goto err;
 
-	if (ibnl_add_client(RDMA_NL_RDMA_CM, RDMA_NL_RDMA_CM_NUM_OPS, cma_cb_table))
+	if (ibnl_add_client(RDMA_NL_RDMA_CM, ARRAY_SIZE(cma_cb_table),
+			    cma_cb_table))
 		pr_warn("RDMA CMA: failed to add netlink callback\n");
 	cma_configfs_init();
 
diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index e28a160..51079e3 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -1175,7 +1175,7 @@ static int __init iw_cm_init(void)
 	if (ret)
 		pr_err("iw_cm: couldn't init iwpm\n");
 
-	ret = ibnl_add_client(RDMA_NL_IWCM, RDMA_NL_IWPM_NUM_OPS,
+	ret = ibnl_add_client(RDMA_NL_IWCM, ARRAY_SIZE(iwcm_nl_cb_table),
 			      iwcm_nl_cb_table);
 	if (ret)
 		pr_err("iw_cm: couldn't register netlink callbacks\n");
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 8a09c0f..1e7c652 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -1820,7 +1820,7 @@ static int __init ib_sa_init(void)
 		goto err3;
 	}
 
-	if (ibnl_add_client(RDMA_NL_LS, RDMA_NL_LS_NUM_OPS,
+	if (ibnl_add_client(RDMA_NL_LS, ARRAY_SIZE(ib_sa_cb_table),
 			    ib_sa_cb_table)) {
 		pr_err("Failed to add netlink callback\n");
 		ret = -EINVAL;
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V2 4/5] IB/SA: Use correct free function

[Leon Romanovsky]

From: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Fixes a direct call to kfree_skb when nlmsg_free should be used.

Fixes: 2ca546b92a02 ('IB/sa: Route SA pathrecord query through netlink')
Signed-off-by: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
---
 drivers/infiniband/core/sa_query.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 1e7c652..3ebd108 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -536,7 +536,7 @@ static int ib_nl_send_msg(struct ib_sa_query *query, gfp_t gfp_mask)
 	data = ibnl_put_msg(skb, &nlh, query->seq, 0, RDMA_NL_LS,
 			    RDMA_NL_LS_OP_RESOLVE, NLM_F_REQUEST);
 	if (!data) {
-		kfree_skb(skb);
+		nlmsg_free(skb);
 		return -EMSGSIZE;
 	}
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

IB address resolution is declared as a module (ib_addr.ko) which loads
itself before IB core module (ib_core.ko).

It causes to the scenario where IB netlink which is initialized by IB
core can't be used by ib_addr.ko.

In order to solve it, we are converting ib_addr.ko to be part of
IB core module.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/infiniband/core/Makefile |  6 ++----
 drivers/infiniband/core/addr.c   | 11 ++---------
 drivers/infiniband/core/device.c | 11 ++++++++++-
 include/rdma/ib_addr.h           |  3 +++
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
index f818538..2c6dc6b 100644
--- a/drivers/infiniband/core/Makefile
+++ b/drivers/infiniband/core/Makefile
@@ -2,7 +2,7 @@ infiniband-$(CONFIG_INFINIBAND_ADDR_TRANS)	:= rdma_cm.o
 user_access-$(CONFIG_INFINIBAND_ADDR_TRANS)	:= rdma_ucm.o
 
 obj-$(CONFIG_INFINIBAND) +=		ib_core.o ib_mad.o ib_sa.o \
-					ib_cm.o iw_cm.o ib_addr.o \
+					ib_cm.o iw_cm.o \
 					$(infiniband-y)
 obj-$(CONFIG_INFINIBAND_USER_MAD) +=	ib_umad.o
 obj-$(CONFIG_INFINIBAND_USER_ACCESS) +=	ib_uverbs.o ib_ucm.o \
@@ -10,7 +10,7 @@ obj-$(CONFIG_INFINIBAND_USER_ACCESS) +=	ib_uverbs.o ib_ucm.o \
 
 ib_core-y :=			packer.o ud_header.o verbs.o cq.o sysfs.o \
 				device.o fmr_pool.o cache.o netlink.o \
-				roce_gid_mgmt.o
+				roce_gid_mgmt.o addr.o
 ib_core-$(CONFIG_INFINIBAND_USER_MEM) += umem.o
 ib_core-$(CONFIG_INFINIBAND_ON_DEMAND_PAGING) += umem_odp.o umem_rbtree.o
 
@@ -28,8 +28,6 @@ rdma_cm-$(CONFIG_INFINIBAND_ADDR_TRANS_CONFIGFS) += cma_configfs.o
 
 rdma_ucm-y :=			ucma.o
 
-ib_addr-y :=			addr.o
-
 ib_umad-y :=			user_mad.o
 
 ib_ucm-y :=			ucm.o
diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index 337353d..3a203ee 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -47,10 +47,6 @@
 #include <rdma/ib_addr.h>
 #include <rdma/ib.h>
 
-MODULE_AUTHOR("Sean Hefty");
-MODULE_DESCRIPTION("IB Address Translation");
-MODULE_LICENSE("Dual BSD/GPL");
-
 struct addr_req {
 	struct list_head list;
 	struct sockaddr_storage src_addr;
@@ -634,7 +630,7 @@ static struct notifier_block nb = {
 	.notifier_call = netevent_callback
 };
 
-static int __init addr_init(void)
+int addr_init(void)
 {
 	addr_wq = create_singlethread_workqueue("ib_addr");
 	if (!addr_wq)
@@ -645,12 +641,9 @@ static int __init addr_init(void)
 	return 0;
 }
 
-static void __exit addr_cleanup(void)
+void addr_cleanup(void)
 {
 	rdma_addr_unregister_client(&self);
 	unregister_netevent_notifier(&nb);
 	destroy_workqueue(addr_wq);
 }
-
-module_init(addr_init);
-module_exit(addr_cleanup);
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 1097984..8894ad0 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -977,16 +977,24 @@ static int __init ib_core_init(void)
 		goto err_comp;
 	}
 
+	ret = addr_init();
+	if (ret) {
+		pr_warn("Could't init IB address resolution\n");
+		goto err_sysfs;
+	}
+
 	ret = ibnl_init();
 	if (ret) {
 		pr_warn("Couldn't init IB netlink interface\n");
-		goto err_sysfs;
+		goto err_addr;
 	}
 
 	ib_cache_setup();
 
 	return 0;
 
+err_addr:
+	addr_cleanup();
 err_sysfs:
 	class_unregister(&ib_class);
 err_comp:
@@ -1000,6 +1008,7 @@ static void __exit ib_core_cleanup(void)
 {
 	ib_cache_cleanup();
 	ibnl_cleanup();
+	addr_cleanup();
 	class_unregister(&ib_class);
 	destroy_workqueue(ib_comp_wq);
 	/* Make sure that any pending umem accounting work is done. */
diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
index 931a47b..e276a07 100644
--- a/include/rdma/ib_addr.h
+++ b/include/rdma/ib_addr.h
@@ -54,6 +54,9 @@ struct rdma_addr_client {
 	struct completion comp;
 };
 
+int addr_init(void);
+void addr_cleanup(void);
+
 /**
  * rdma_addr_register_client - Register an address client.
  */
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

On 05/06/2016 03:45 PM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> IB address resolution is declared as a module (ib_addr.ko) which loads
> itself before IB core module (ib_core.ko).
> 
> It causes to the scenario where IB netlink which is initialized by IB
> core can't be used by ib_addr.ko.
> 
> In order to solve it, we are converting ib_addr.ko to be part of
> IB core module.

> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 1097984..8894ad0 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -977,16 +977,24 @@ static int __init ib_core_init(void)
>  		goto err_comp;
>  	}
>  
> +	ret = addr_init();
> +	if (ret) {
> +		pr_warn("Could't init IB address resolution\n");
> +		goto err_sysfs;
> +	}
> +
>  	ret = ibnl_init();
>  	if (ret) {
>  		pr_warn("Couldn't init IB netlink interface\n");
> -		goto err_sysfs;
> +		goto err_addr;
>  	}

It's unclear to me how this change helps things.  Theoretically, if you
load ib_addr before ib_core (and therefore before any ibnl_init() is
run) and that prevents ib_addr from using the ibnl infrastructure, then
you would think that building ib_addr into ib_core and then running the
addr_init() before the ibnl_init() would have exactly the same problem.
I don't see what issue is resolved by this patch.  But, just as
importantly, after reading addr.c to see how it uses the ibnl
infrastructure, I don't even see what the original problem can be.
Please elaborate more on what actual problem we are solving with this
because I am loathe to change the module structure without good cause
(in particular, I know at least the Red Hat init scripts and systemd
units know about module names, so this requires changes to user space
scripts and that's a big no-no if it can be avoided).


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Doug Ledford
> Sent: Friday, May 13, 2016 10:34 PM
> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
> On 05/06/2016 03:45 PM, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > IB address resolution is declared as a module (ib_addr.ko) which loads
> > itself before IB core module (ib_core.ko).
> >
> > It causes to the scenario where IB netlink which is initialized by IB
> > core can't be used by ib_addr.ko.
> >
> > In order to solve it, we are converting ib_addr.ko to be part of
> > IB core module.
> 
> > diff --git a/drivers/infiniband/core/device.c
> b/drivers/infiniband/core/device.c
> > index 1097984..8894ad0 100644
> > --- a/drivers/infiniband/core/device.c
> > +++ b/drivers/infiniband/core/device.c
> > @@ -977,16 +977,24 @@ static int __init ib_core_init(void)
> >  		goto err_comp;
> >  	}
> >
> > +	ret = addr_init();
> > +	if (ret) {
> > +		pr_warn("Could't init IB address resolution\n");
> > +		goto err_sysfs;
> > +	}
> > +
> >  	ret = ibnl_init();
> >  	if (ret) {
> >  		pr_warn("Couldn't init IB netlink interface\n");
> > -		goto err_sysfs;
> > +		goto err_addr;
> >  	}
> 
> It's unclear to me how this change helps things.  Theoretically, if you
> load ib_addr before ib_core (and therefore before any ibnl_init() is
> run) and that prevents ib_addr from using the ibnl infrastructure, then
> you would think that building ib_addr into ib_core and then running the
> addr_init() before the ibnl_init() would have exactly the same problem.
> I don't see what issue is resolved by this patch.
You are right, this patch doesn't solve the problem of using ibnl inside ib_addr,
But this patch didn't try to do that, all it did was to integrate ib_addr into ib_core,
It tried to preserve the current order of dependencies . 

If this change is accepted, the order will be switched in the patches that add
ibnl usage inside addr.c, another option is to pull this patch with the correct order
into that series, this way we won't have to touch this area twice.

> But, just as importantly, after reading addr.c to see how it uses the ibnl
> infrastructure, I don't even see what the original problem can be.
As it stands today:
- ibnl is part of ib_core.
- ib_core needs ib_addr.

So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
which causes a dependency cycle.

> Please elaborate more on what actual problem we are solving with this
> because I am loathe to change the module structure without good cause
> (in particular, I know at least the Red Hat init scripts and systemd
> units know about module names, so this requires changes to user space
> scripts and that's a big no-no if it can be avoided).
> 
> 
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 


Mark Bloch.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On 05/15/2016 06:51 AM, Mark Bloch wrote:

>> But, just as importantly, after reading addr.c to see how it uses the ibnl
>> infrastructure, I don't even see what the original problem can be.
> As it stands today:
> - ibnl is part of ib_core.
> - ib_core needs ib_addr.
> 
> So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
> which causes a dependency cycle.

Right, but in that case, this patch needs to be part of the series that
adds the ibnl support into the ib_addr functionality.  Because you split
them into separate series, this was a patch looking for a problem to
solve and it wasn't clear what it was.  If I had taken the other series
and not this series, it would have broken things.  So please keep
patches like this together with the other patches that depend on it.

That said, I also don't want to redo modules if we don't have to.  As my
previous email points out, changing modules breaks init scripts and
systemd unit files.  It is to be avoided when possible.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1486 bytes --]

On Mon, May 16, 2016 at 11:09:44AM -0400, Doug Ledford wrote:
> On 05/15/2016 06:51 AM, Mark Bloch wrote:
> 
> >> But, just as importantly, after reading addr.c to see how it uses the ibnl
> >> infrastructure, I don't even see what the original problem can be.
> > As it stands today:
> > - ibnl is part of ib_core.
> > - ib_core needs ib_addr.
> > 
> > So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
> > which causes a dependency cycle.
> 
> Right, but in that case, this patch needs to be part of the series that
> adds the ibnl support into the ib_addr functionality.  Because you split
> them into separate series, this was a patch looking for a problem to
> solve and it wasn't clear what it was.  If I had taken the other series
> and not this series, it would have broken things.  So please keep
> patches like this together with the other patches that depend on it.
> 
> That said, I also don't want to redo modules if we don't have to.  As my
> previous email points out, changing modules breaks init scripts and
> systemd unit files.  It is to be avoided when possible.

Sorry,
I was in the mood of fixing things when I wrote and sent this patch.
The question is which version will you more likely to accept: this one
(remove ib_addr module) or previous one (add ib_netlink module)?

Thanks.
> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On 05/16/2016 12:30 PM, Leon Romanovsky wrote:
> On Mon, May 16, 2016 at 11:09:44AM -0400, Doug Ledford wrote:
>> On 05/15/2016 06:51 AM, Mark Bloch wrote:
>>
>>>> But, just as importantly, after reading addr.c to see how it uses the ibnl
>>>> infrastructure, I don't even see what the original problem can be.
>>> As it stands today:
>>> - ibnl is part of ib_core.
>>> - ib_core needs ib_addr.
>>>
>>> So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
>>> which causes a dependency cycle.
>>
>> Right, but in that case, this patch needs to be part of the series that
>> adds the ibnl support into the ib_addr functionality.  Because you split
>> them into separate series, this was a patch looking for a problem to
>> solve and it wasn't clear what it was.  If I had taken the other series
>> and not this series, it would have broken things.  So please keep
>> patches like this together with the other patches that depend on it.
>>
>> That said, I also don't want to redo modules if we don't have to.  As my
>> previous email points out, changing modules breaks init scripts and
>> systemd unit files.  It is to be avoided when possible.
> 
> Sorry,
> I was in the mood of fixing things when I wrote and sent this patch.
> The question is which version will you more likely to accept: this one
> (remove ib_addr module) or previous one (add ib_netlink module)?

Can you build netlink in and then init the ib_addr module after the
netlink init is complete?  Wouldn't that resolve the dependency ordering
issue without changing the module names?


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Jason Gunthorpe]

On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
> Can you build netlink in and then init the ib_addr module after the
> netlink init is complete?  Wouldn't that resolve the dependency ordering
> issue without changing the module names?

Why are you so worried about ib_addr's module name? Is that actually
hand loaded instead of relying in the implicit dependencies???

Is failure to load a module going to break any existing scripts?

Worse comes to worse, just make dummy empty ib_addr.ko, but
I've never seen that in-kernel before.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]

On 05/16/2016 02:27 PM, Jason Gunthorpe wrote:
> On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
>> Can you build netlink in and then init the ib_addr module after the
>> netlink init is complete?  Wouldn't that resolve the dependency ordering
>> issue without changing the module names?
> 
> Why are you so worried about ib_addr's module name? Is that actually
> hand loaded instead of relying in the implicit dependencies???

It's hand unloaded.  Depending on the release of software you are
talking about, the init script has the ability to unload/reload the IB
stack.  In that case, it has to know about ib_addr so that it can unload
after ib_core.  I should know, when the order changed from ib_core first
and ib_addr second to the opposite it broke the Red Hat init script for
a release, including all the bug reports about the rdma stack failing to
shut down properly because it was unloading modules in the wrong order.
This falls in the same category of a lot of other things: you don't
break user space if you don't have to.

> Is failure to load a module going to break any existing scripts?
> 
> Worse comes to worse, just make dummy empty ib_addr.ko, but
> I've never seen that in-kernel before.
> 
> Jason
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1913 bytes --]

On Mon, May 16, 2016 at 02:39:10PM -0400, Doug Ledford wrote:
> On 05/16/2016 02:27 PM, Jason Gunthorpe wrote:
> > On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
> >> Can you build netlink in and then init the ib_addr module after the
> >> netlink init is complete?  Wouldn't that resolve the dependency ordering
> >> issue without changing the module names?
> > 
> > Why are you so worried about ib_addr's module name? Is that actually
> > hand loaded instead of relying in the implicit dependencies???
> 
> It's hand unloaded.  Depending on the release of software you are
> talking about, the init script has the ability to unload/reload the IB
> stack.  In that case, it has to know about ib_addr so that it can unload
> after ib_core.  I should know, when the order changed from ib_core first
> and ib_addr second to the opposite it broke the Red Hat init script for
> a release, including all the bug reports about the rdma stack failing to
> shut down properly because it was unloading modules in the wrong order.
> This falls in the same category of a lot of other things: you don't
> break user space if you don't have to.

I think the reason that this thread fails to die is our trouble to
understand if code refactoring is enough justification to get rid of all
these modules.

All guides/scripts which I saw, instructs to load all these ib_...
modules together and it looks redundant to have such large number of
them [1].

[1]
http://www.rdmamojo.com/2014/11/08/working-rdma-ubuntu/#Starting_the_RDMA_services

> 
> > Is failure to load a module going to break any existing scripts?
> > 
> > Worse comes to worse, just make dummy empty ib_addr.ko, but
> > I've never seen that in-kernel before.
> > 
> > Jason
> > 
> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

On 05/18/2016 12:41 AM, Leon Romanovsky wrote:
> On Mon, May 16, 2016 at 02:39:10PM -0400, Doug Ledford wrote:
>> On 05/16/2016 02:27 PM, Jason Gunthorpe wrote:
>>> On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
>>>> Can you build netlink in and then init the ib_addr module after the
>>>> netlink init is complete?  Wouldn't that resolve the dependency ordering
>>>> issue without changing the module names?
>>>
>>> Why are you so worried about ib_addr's module name? Is that actually
>>> hand loaded instead of relying in the implicit dependencies???
>>
>> It's hand unloaded.  Depending on the release of software you are
>> talking about, the init script has the ability to unload/reload the IB
>> stack.  In that case, it has to know about ib_addr so that it can unload
>> after ib_core.  I should know, when the order changed from ib_core first
>> and ib_addr second to the opposite it broke the Red Hat init script for
>> a release, including all the bug reports about the rdma stack failing to
>> shut down properly because it was unloading modules in the wrong order.
>> This falls in the same category of a lot of other things: you don't
>> break user space if you don't have to.
> 
> I think the reason that this thread fails to die is our trouble to
> understand if code refactoring is enough justification to get rid of all
> these modules.
> 
> All guides/scripts which I saw, instructs to load all these ib_...
> modules together and it looks redundant to have such large number of
> them [1].
> 
> [1]
> http://www.rdmamojo.com/2014/11/08/working-rdma-ubuntu/#Starting_the_RDMA_services

I've never heard of rdmamojo before...interesting.

Anyway, it probably is true that the number of modules is superfluous.
I'm not arguing against that.

>>
>>> Is failure to load a module going to break any existing scripts?
>>>
>>> Worse comes to worse, just make dummy empty ib_addr.ko, but
>>> I've never seen that in-kernel before.
>>>
>>> Jason
>>>
>>
>>
>> -- 
>> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>               GPG KeyID: 0E572FDD
>>
>>
> 
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
> On 05/16/2016 12:30 PM, Leon Romanovsky wrote:
> > On Mon, May 16, 2016 at 11:09:44AM -0400, Doug Ledford wrote:
> >> On 05/15/2016 06:51 AM, Mark Bloch wrote:
> >>
> >>>> But, just as importantly, after reading addr.c to see how it uses the ibnl
> >>>> infrastructure, I don't even see what the original problem can be.
> >>> As it stands today:
> >>> - ibnl is part of ib_core.
> >>> - ib_core needs ib_addr.
> >>>
> >>> So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
> >>> which causes a dependency cycle.
> >>
> >> Right, but in that case, this patch needs to be part of the series that
> >> adds the ibnl support into the ib_addr functionality.  Because you split
> >> them into separate series, this was a patch looking for a problem to
> >> solve and it wasn't clear what it was.  If I had taken the other series
> >> and not this series, it would have broken things.  So please keep
> >> patches like this together with the other patches that depend on it.
> >>
> >> That said, I also don't want to redo modules if we don't have to.  As my
> >> previous email points out, changing modules breaks init scripts and
> >> systemd unit files.  It is to be avoided when possible.
> > 
> > Sorry,
> > I was in the mood of fixing things when I wrote and sent this patch.
> > The question is which version will you more likely to accept: this one
> > (remove ib_addr module) or previous one (add ib_netlink module)?
> 
> Can you build netlink in and then init the ib_addr module after the
> netlink init is complete?  Wouldn't that resolve the dependency ordering
> issue without changing the module names?

It seems reasonable and we will test it, before reposting.

However generally speaking, I agree with Jason and Ira that this module
(ib_addr.ko) is useless as module and can be part of ib_core.ko.

It doesn't seem as a big deal to fix all that init scripts (remove two
lines).

> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2698 bytes --]

On 05/16/2016 02:54 PM, Leon Romanovsky wrote:
> On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
>> On 05/16/2016 12:30 PM, Leon Romanovsky wrote:
>>> On Mon, May 16, 2016 at 11:09:44AM -0400, Doug Ledford wrote:
>>>> On 05/15/2016 06:51 AM, Mark Bloch wrote:
>>>>
>>>>>> But, just as importantly, after reading addr.c to see how it uses the ibnl
>>>>>> infrastructure, I don't even see what the original problem can be.
>>>>> As it stands today:
>>>>> - ibnl is part of ib_core.
>>>>> - ib_core needs ib_addr.
>>>>>
>>>>> So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
>>>>> which causes a dependency cycle.
>>>>
>>>> Right, but in that case, this patch needs to be part of the series that
>>>> adds the ibnl support into the ib_addr functionality.  Because you split
>>>> them into separate series, this was a patch looking for a problem to
>>>> solve and it wasn't clear what it was.  If I had taken the other series
>>>> and not this series, it would have broken things.  So please keep
>>>> patches like this together with the other patches that depend on it.
>>>>
>>>> That said, I also don't want to redo modules if we don't have to.  As my
>>>> previous email points out, changing modules breaks init scripts and
>>>> systemd unit files.  It is to be avoided when possible.
>>>
>>> Sorry,
>>> I was in the mood of fixing things when I wrote and sent this patch.
>>> The question is which version will you more likely to accept: this one
>>> (remove ib_addr module) or previous one (add ib_netlink module)?
>>
>> Can you build netlink in and then init the ib_addr module after the
>> netlink init is complete?  Wouldn't that resolve the dependency ordering
>> issue without changing the module names?
> 
> It seems reasonable and we will test it, before reposting.

Thanks.

> However generally speaking, I agree with Jason and Ira that this module
> (ib_addr.ko) is useless as module and can be part of ib_core.ko.
> 
> It doesn't seem as a big deal to fix all that init scripts (remove two
> lines).

It's not removing two lines.  If someone wants to be able to boot both
kernels prior to this change and kernel post this change, they have to
now write their script to handle both conditions sanely.  And they have
to know to do so, where as very often they simply get caught off guard
and things are broken for some period of time.  If the change to ib_addr
actually solved a problem that couldn't be solved otherwise, that would
be one thing.  But it doesn't.  It's a complete no-op.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Jason Gunthorpe]

> and things are broken for some period of time.  If the change to ib_addr
> actually solved a problem that couldn't be solved otherwise, that would
> be one thing.  But it doesn't.  It's a complete no-op.

The original patches had a horrible dynamic registration scheme for
the netlink dispatch that only served to work around the pointless
module split. The issue is that the dispatch needs to have function
relocations from both modules to follow the usual netlink idioms.

It is unfortunate that distro scripts have become such that they break
if the kernel changes even something small like dependencies.

Another alternative is to move only the parts of ib_addr that touch
netlink into ib_core. Hopefully that doesn't create an empty module.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Tuesday, May 17, 2016 12:03 AM
> To: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
> > and things are broken for some period of time.  If the change to ib_addr
> > actually solved a problem that couldn't be solved otherwise, that would
> > be one thing.  But it doesn't.  It's a complete no-op.
> 
> The original patches had a horrible dynamic registration scheme for
> the netlink dispatch that only served to work around the pointless
> module split. The issue is that the dispatch needs to have function
> relocations from both modules to follow the usual netlink idioms.
Are you talking about  the changes in ibnl_add_client?
Those changes were because of a different module (ib_sa.ko)
In sa_query.c we register a ibnl client for RDMA_NL_LS and
we use that for getting path records (ibacm runs in userspace and listens 
on that netlink socket). If we look at the definition of RDMA_NS_LS
it is defined as a  rdma local service operations, so naturedly
I feel  ip->gid should be added to that family.

It means we have two modules (ib_sa.ko and ib_core/ib_addr depends if we integrate them or not)
Needing to use different operations which are part of the same family.

The approach I've chosen is the easiest to implement because I'm not sure we
need all the complexity other solutions impose. I think Ira suggested to introduce "add operation"
it's an idea I've thought about, but it raises the issue of ibnl needing to manage each family.
This idea has the benefit if we want to have an overlap between clients, can you think
of a use case where different clients need to support the same operation?

I should also mention the option of adding a new family RDMA_NL_LS_IP or something 
like that. I didn't do that because we are using ibacm to answer the ip->gid requests.
Ibacm already listens on a netlink socket for RDMA_NS_LS, so it makes adding
Ip->gid to ibacm much easier and nicer if it needs to listen on only one netlink socket.

> It is unfortunate that distro scripts have become such that they break
> if the kernel changes even something small like dependencies.
> 
> Another alternative is to move only the parts of ib_addr that touch
> netlink into ib_core. Hopefully that doesn't create an empty module.
> 
> Jason

Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Jason Gunthorpe]

On Tue, May 17, 2016 at 05:00:03AM +0000, Mark Bloch wrote:

> Are you talking about  the changes in ibnl_add_client?
> Those changes were because of a different module (ib_sa.ko)

Bleck, why is this such a mess? Can't you get everything into one
module so we don't need this dynamic stuff??

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Jason Gunthorpe [mailto:jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org]
> Sent: Tuesday, May 17, 2016 7:57 PM
> To: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
> On Tue, May 17, 2016 at 05:00:03AM +0000, Mark Bloch wrote:
> 
> > Are you talking about  the changes in ibnl_add_client?
> > Those changes were because of a different module (ib_sa.ko)
> 
> Bleck, why is this such a mess? Can't you get everything into one
> module so we don't need this dynamic stuff??
Well, seeing as ib_sa.ko doesn't have to be loaded we will still need
A dynamic part somewhere, to handle the registration/deregistration.
We will probably need a new module to manage that.
I think this is doable, but do we need it?

I agree there might be a more elegant solution, but I don't see any other
modules that will use it.  Do we have a use case other than ib_sa/ib_core?

Mark

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Christoph Hellwig]

On Mon, May 16, 2016 at 03:03:27PM -0600, Jason Gunthorpe wrote:
> It is unfortunate that distro scripts have become such that they break
> if the kernel changes even something small like dependencies.

What script exactly break, btw?  We had quite a few module merges /
split in other subsystems I contribute to, and they've not been major
issues.


> Another alternative is to move only the parts of ib_addr that touch
> netlink into ib_core. Hopefully that doesn't create an empty module.

Or just create a dummy ib_addr module IFF it really is an issue.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Mon, May 16, 2016 at 10:46:52PM -0700, Christoph Hellwig wrote:
> On Mon, May 16, 2016 at 03:03:27PM -0600, Jason Gunthorpe wrote:
> > It is unfortunate that distro scripts have become such that they break
> > if the kernel changes even something small like dependencies.
> 
> What script exactly break, btw?  We had quite a few module merges /
> split in other subsystems I contribute to, and they've not been major
> issues.

Debian Sid works like a charm with/without ib_addr.ko as a separate
module. It looks like breakage will be in testing scripts only.

> 
> 
> > Another alternative is to move only the parts of ib_addr that touch
> > netlink into ib_core. Hopefully that doesn't create an empty module.
> 
> Or just create a dummy ib_addr module IFF it really is an issue.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 3231 bytes --]

On 05/17/2016 01:46 AM, Christoph Hellwig wrote:
> On Mon, May 16, 2016 at 03:03:27PM -0600, Jason Gunthorpe wrote:
>> It is unfortunate that distro scripts have become such that they break
>> if the kernel changes even something small like dependencies.
> 
> What script exactly break, btw?  We had quite a few module merges /
> split in other subsystems I contribute to, and they've not been major
> issues.

The Red Hat SysV Init script for the RDMA subsystem on both RHEL and
Fedora (prior to the switch over to systemd for the RDMA init, which
means EL6 and earlier and I'm not sure when we switched in Fedora, but
I'm sure the old SysV init version of the Fedora rdma package is already
EOL) and the OFED SysV init script are the two for certain that I know of.

The RDMA subsystem is a bit unique in that the older SysV init scripts
allowed a reload operation, and that required removing all the modules
and then reinserting them.  The more modern systemd unit did away with
reload as an option and so it would work fine with the module name
change (although it would barf on other more important modules if they
changed names, primarily the end modules, such as ucma and friends).

I can't speak to Debian.  Among other issues, the only reason this would
even possibly be an issue for seemingly older OS releases is if the OS
in question still does either an RDMA stack update in a stable kernel
(like Red Hat does with all of its releases, including EL 6) or if the
older release gets newer kernels (like we do with Fedora, but our Fedora
releases short lived and so this isn't a problem there, but I have no
knowledge of Debian releases).

> 
>> Another alternative is to move only the parts of ib_addr that touch
>> netlink into ib_core. Hopefully that doesn't create an empty module.
> 
> Or just create a dummy ib_addr module IFF it really is an issue.

This is an awful lot of arguing over seemingly nothing.  These things
were true when I rejected the first patch:

1) It didn't solve any problem.  By code inspection, it wouldn't even
work to solve the problem that the commit message says it was supposed
to solve.
2) It was part of a series that was unrelated to the issue it was
supposed to help instead of part of the series that supposedly needed it.
3) It needlessly broke user space scripts (for systems that get updated
to modern upstream, which, believe it or not, EL 6 still is included in
that...the EL 6.8 update that just went out includes an upstream 4.1
RDMA core subsystem).

If you want me to take the patch, then one of 1? and 2 need to be true:

1a) It needs to actually solve a problem in the patch itself (and the
git log should reflect the problem it solves).

1b) It can solve a problem for some other code in the same series as it.

2) The solution must at least be more elegant or appropriate than
alternative solutions that don't break user space.

In short, give me actual justification and I'll take it.  After all,
breaking older init scripts is a pretty minor user space break.  But I
need *some* justification.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3199 bytes --]

On Mon, May 16, 2016 at 04:27:05PM -0400, Doug Ledford wrote:
> On 05/16/2016 02:54 PM, Leon Romanovsky wrote:
> > On Mon, May 16, 2016 at 01:42:34PM -0400, Doug Ledford wrote:
> >> On 05/16/2016 12:30 PM, Leon Romanovsky wrote:
> >>> On Mon, May 16, 2016 at 11:09:44AM -0400, Doug Ledford wrote:
> >>>> On 05/15/2016 06:51 AM, Mark Bloch wrote:
> >>>>
> >>>>>> But, just as importantly, after reading addr.c to see how it uses the ibnl
> >>>>>> infrastructure, I don't even see what the original problem can be.
> >>>>> As it stands today:
> >>>>> - ibnl is part of ib_core.
> >>>>> - ib_core needs ib_addr.
> >>>>>
> >>>>> So if we add ibnl usage to ib_addr it means ib_addr will need ib_core,
> >>>>> which causes a dependency cycle.
> >>>>
> >>>> Right, but in that case, this patch needs to be part of the series that
> >>>> adds the ibnl support into the ib_addr functionality.  Because you split
> >>>> them into separate series, this was a patch looking for a problem to
> >>>> solve and it wasn't clear what it was.  If I had taken the other series
> >>>> and not this series, it would have broken things.  So please keep
> >>>> patches like this together with the other patches that depend on it.
> >>>>
> >>>> That said, I also don't want to redo modules if we don't have to.  As my
> >>>> previous email points out, changing modules breaks init scripts and
> >>>> systemd unit files.  It is to be avoided when possible.
> >>>
> >>> Sorry,
> >>> I was in the mood of fixing things when I wrote and sent this patch.
> >>> The question is which version will you more likely to accept: this one
> >>> (remove ib_addr module) or previous one (add ib_netlink module)?
> >>
> >> Can you build netlink in and then init the ib_addr module after the
> >> netlink init is complete?  Wouldn't that resolve the dependency ordering
> >> issue without changing the module names?
> > 
> > It seems reasonable and we will test it, before reposting.
> 
> Thanks.
> 
> > However generally speaking, I agree with Jason and Ira that this module
> > (ib_addr.ko) is useless as module and can be part of ib_core.ko.
> > 
> > It doesn't seem as a big deal to fix all that init scripts (remove two
> > lines).
> 
> It's not removing two lines.  If someone wants to be able to boot both
> kernels prior to this change and kernel post this change, they have to
> now write their script to handle both conditions sanely.  And they have
> to know to do so, where as very often they simply get caught off guard
> and things are broken for some period of time.  If the change to ib_addr
> actually solved a problem that couldn't be solved otherwise, that would
> be one thing.  But it doesn't.  It's a complete no-op.

I think that your position is too tight for not allowing modules
restructure. It looks like that this complexity doesn't exist outside
testing scripts.

For my testings, I'm using Debian Sid with inbox scripts/packages and it handles
correctly IB stack with different kernels and different set of modules.

> 
> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1179 bytes --]

On 05/17/2016 01:48 AM, Leon Romanovsky wrote:
> On Mon, May 16, 2016 at 04:27:05PM -0400, Doug Ledford wrote:

>> It's not removing two lines.  If someone wants to be able to boot both
>> kernels prior to this change and kernel post this change, they have to
>> now write their script to handle both conditions sanely.  And they have
>> to know to do so, where as very often they simply get caught off guard
>> and things are broken for some period of time.  If the change to ib_addr
>> actually solved a problem that couldn't be solved otherwise, that would
>> be one thing.  But it doesn't.  It's a complete no-op.
> 
> I think that your position is too tight for not allowing modules
> restructure. It looks like that this complexity doesn't exist outside
> testing scripts.

No, they were/are production scripts.

> For my testings, I'm using Debian Sid with inbox scripts/packages and it handles
> correctly IB stack with different kernels and different set of modules.

Does it support reloading the stack and have you tried it if it does?

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On Tue, May 17, 2016 at 10:58:19AM -0400, Doug Ledford wrote:
> On 05/17/2016 01:48 AM, Leon Romanovsky wrote:
> > On Mon, May 16, 2016 at 04:27:05PM -0400, Doug Ledford wrote:
> 
> >> It's not removing two lines.  If someone wants to be able to boot both
> >> kernels prior to this change and kernel post this change, they have to
> >> now write their script to handle both conditions sanely.  And they have
> >> to know to do so, where as very often they simply get caught off guard
> >> and things are broken for some period of time.  If the change to ib_addr
> >> actually solved a problem that couldn't be solved otherwise, that would
> >> be one thing.  But it doesn't.  It's a complete no-op.
> > 
> > I think that your position is too tight for not allowing modules
> > restructure. It looks like that this complexity doesn't exist outside
> > testing scripts.
> 
> No, they were/are production scripts.
> 
> > For my testings, I'm using Debian Sid with inbox scripts/packages and it handles
> > correctly IB stack with different kernels and different set of modules.
> 
> Does it support reloading the stack and have you tried it if it does?

It is RedHat specific, from my understanding in other distribution there is no
special script to reload RDMA, I wrote for myself module reload script
for debug.

> 
> -- 
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 
> 



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

On 05/18/2016 12:47 AM, Leon Romanovsky wrote:
> On Tue, May 17, 2016 at 10:58:19AM -0400, Doug Ledford wrote:
>> On 05/17/2016 01:48 AM, Leon Romanovsky wrote:
>>> On Mon, May 16, 2016 at 04:27:05PM -0400, Doug Ledford wrote:
>>
>>>> It's not removing two lines.  If someone wants to be able to boot both
>>>> kernels prior to this change and kernel post this change, they have to
>>>> now write their script to handle both conditions sanely.  And they have
>>>> to know to do so, where as very often they simply get caught off guard
>>>> and things are broken for some period of time.  If the change to ib_addr
>>>> actually solved a problem that couldn't be solved otherwise, that would
>>>> be one thing.  But it doesn't.  It's a complete no-op.
>>>
>>> I think that your position is too tight for not allowing modules
>>> restructure. It looks like that this complexity doesn't exist outside
>>> testing scripts.
>>
>> No, they were/are production scripts.
>>
>>> For my testings, I'm using Debian Sid with inbox scripts/packages and it handles
>>> correctly IB stack with different kernels and different set of modules.
>>
>> Does it support reloading the stack and have you tried it if it does?
> 
> It is RedHat specific, from my understanding in other distribution there is no
> special script to reload RDMA, I wrote for myself module reload script
> for debug.

Then I guess I was just nice to developers back in the day.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Monday, May 16, 2016 8:43 PM
> To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> Can you build netlink in and then init the ib_addr module after the
> netlink init is complete?  Wouldn't that resolve the dependency ordering
> issue without changing the module names?
Sorry, but I don't understand what do you mean by this.
If you would like to keep the current module structure (ib_core.ko and ib_addr.ko)
The only way to use ibnl from within addr.c is to move the ibnl initialization to addr.c
and build netlink.c as part of ib_addr.ko, but it seems kind of strange if
ib_addr is responsible to initialize ibnl.

> 
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 

Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 5591 bytes --]

On 05/17/2016 04:29 AM, Mark Bloch wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>> Sent: Monday, May 16, 2016 8:43 PM
>> To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
>> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
>> resolution module into core
>> Can you build netlink in and then init the ib_addr module after the
>> netlink init is complete?  Wouldn't that resolve the dependency ordering
>> issue without changing the module names?
> Sorry, but I don't understand what do you mean by this.
> If you would like to keep the current module structure (ib_core.ko and ib_addr.ko)
> The only way to use ibnl from within addr.c is to move the ibnl initialization to addr.c
> and build netlink.c as part of ib_addr.ko, but it seems kind of strange if
> ib_addr is responsible to initialize ibnl.

Not according to what I was looking at.  In the original patch, you
moved addr.c into ib_core and changed addr_init and addr_cleanup from
static to normal.  You were then able to control whether the addr code
went before or after the netlink code by rearranging the order of init
sequences in device.c:ib_core_init().  It is entirely possible that you
could leave ib_addr as a module, change addr_init to rdma_addr_init and
EXPORT_SYMBOL it, and likewise with the cleanup function, and remove the
module_init(addr_init); and module_exit(addr_cleanup); from the addr.c
file.  This would then allow you to control when ib_addr was inited by
adding a call to rdma_addr_init() in device.c:ib_core_init() just like
you had in your patch.  It would solve the problem and give you complete
control while being transparent to user space.  This is what I mean:

commit ec5394412286e325b83b6c64d026f4d7bab40ccd
Author: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Tue May 17 11:43:57 2016 -0400

    IB/core: change module init ordering

    Change ib_addr to be init'ed by ib_core and not by the module load
    process.  This gives us precise controle of when the module is
    initialized while still preserving the existing module names and load
    ordering.  This is to be backward compatible with existing, older
    SysV init scripts.  When those older systems are dead and gone, we
    can revisit this and change the module names and load orders however
    we want.

    Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
index 337353d86cfa..fda808546e0e 100644
--- a/drivers/infiniband/core/addr.c
+++ b/drivers/infiniband/core/addr.c
@@ -634,7 +634,17 @@ static struct notifier_block nb = {
        .notifier_call = netevent_callback
 };

-static int __init addr_init(void)
+/*
+ * This is a little unorthodox.  We export our init function so that the
+ * ib_core module can call it at the right time after the other modules
+ * we depend on have registered.  We do this solely because there are some
+ * user space init scripts that expect ib_addr to be loaded prior to
ib_core
+ * and due to recent changes, we need the ib netlink code brought up before
+ * we bring this code up and that code is now in ib_core.  When the older
+ * SysV init script systems that have these init scripts are dead and gone,
+ * we can revisit this and possibly just include addr.c in ib_core.
+ */
+int rdma_addr_init(void)
 {
        addr_wq = create_singlethread_workqueue("ib_addr");
        if (!addr_wq)
@@ -644,13 +654,12 @@ static int __init addr_init(void)
        rdma_addr_register_client(&self);
        return 0;
 }
+EXPORT_SYMBOL(rdma_addr_init);

-static void __exit addr_cleanup(void)
+void rdma_addr_cleanup(void)
 {
        rdma_addr_unregister_client(&self);
        unregister_netevent_notifier(&nb);
        destroy_workqueue(addr_wq);
 }
-
-module_init(addr_init);
-module_exit(addr_cleanup);
+EXPORT_SYMBOL(rdma_addr_cleanup);
diff --git a/drivers/infiniband/core/core_priv.h
b/drivers/infiniband/core/core_priv.h
index eab32215756b..e0a5187ea98c 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -137,4 +137,7 @@ static inline bool rdma_is_upper_dev_rcu(struct
net_device *dev,
        return _upper == upper;
 }

+int rdma_addr_init(void);
+void rdma_addr_cleanup(void);
+
 #endif /* _CORE_PRIV_H */
diff --git a/drivers/infiniband/core/device.c
b/drivers/infiniband/core/device.c
index 10979844026a..4fdf87a485cc 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -983,10 +983,18 @@ static int __init ib_core_init(void)
                goto err_sysfs;
        }

+       ret = rdma_addr_init();
+       if (ret) {
+               pr_warn("Couldn't init IB address resolution module\n");
+               goto err_ibnl;
+       }
+
        ib_cache_setup();

        return 0;

+err_ibnl:
+       ibnl_cleanup();
 err_sysfs:
        class_unregister(&ib_class);
 err_comp:
@@ -999,6 +1007,7 @@ err:
 static void __exit ib_core_cleanup(void)
 {
        ib_cache_cleanup();
+       rdma_addr_cleanup();
        ibnl_cleanup();
        class_unregister(&ib_class);
        destroy_workqueue(ib_comp_wq);



-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Tuesday, May 17, 2016 6:49 PM
> To: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
> On 05/17/2016 04:29 AM, Mark Bloch wrote:
> >
> >
> >> -----Original Message-----
> >> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> >> Sent: Monday, May 16, 2016 8:43 PM
> >> To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> >> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> >> resolution module into core
> >> Can you build netlink in and then init the ib_addr module after the
> >> netlink init is complete?  Wouldn't that resolve the dependency ordering
> >> issue without changing the module names?
> > Sorry, but I don't understand what do you mean by this.
> > If you would like to keep the current module structure (ib_core.ko and
> ib_addr.ko)
> > The only way to use ibnl from within addr.c is to move the ibnl initialization
> to addr.c
> > and build netlink.c as part of ib_addr.ko, but it seems kind of strange if
> > ib_addr is responsible to initialize ibnl.
> 
> Not according to what I was looking at.  In the original patch, you
> moved addr.c into ib_core and changed addr_init and addr_cleanup from
> static to normal.  You were then able to control whether the addr code
> went before or after the netlink code by rearranging the order of init
> sequences in device.c:ib_core_init().  It is entirely possible that you
> could leave ib_addr as a module, change addr_init to rdma_addr_init and
> EXPORT_SYMBOL it, and likewise with the cleanup function, and remove the
> module_init(addr_init); and module_exit(addr_cleanup); from the addr.c
> file.  This would then allow you to control when ib_addr was inited by
> adding a call to rdma_addr_init() in device.c:ib_core_init() just like
> you had in your patch.  It would solve the problem and give you complete
> control while being transparent to user space.  This is what I mean:
> 
> commit ec5394412286e325b83b6c64d026f4d7bab40ccd
> Author: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date:   Tue May 17 11:43:57 2016 -0400
> 
>     IB/core: change module init ordering
> 
>     Change ib_addr to be init'ed by ib_core and not by the module load
>     process.  This gives us precise controle of when the module is
>     initialized while still preserving the existing module names and load
>     ordering.  This is to be backward compatible with existing, older
>     SysV init scripts.  When those older systems are dead and gone, we
>     can revisit this and change the module names and load orders however
>     we want.
> 
>     Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
> index 337353d86cfa..fda808546e0e 100644
> --- a/drivers/infiniband/core/addr.c
> +++ b/drivers/infiniband/core/addr.c
> @@ -634,7 +634,17 @@ static struct notifier_block nb = {
>         .notifier_call = netevent_callback
>  };
> 
> -static int __init addr_init(void)
> +/*
> + * This is a little unorthodox.  We export our init function so that the
> + * ib_core module can call it at the right time after the other modules
> + * we depend on have registered.  We do this solely because there are
> some
> + * user space init scripts that expect ib_addr to be loaded prior to
> ib_core
> + * and due to recent changes, we need the ib netlink code brought up
> before
> + * we bring this code up and that code is now in ib_core.  When the older
> + * SysV init script systems that have these init scripts are dead and gone,
> + * we can revisit this and possibly just include addr.c in ib_core.
> + */
> +int rdma_addr_init(void)
>  {
>         addr_wq = create_singlethread_workqueue("ib_addr");
>         if (!addr_wq)
> @@ -644,13 +654,12 @@ static int __init addr_init(void)
>         rdma_addr_register_client(&self);
>         return 0;
>  }
> +EXPORT_SYMBOL(rdma_addr_init);
> 
> -static void __exit addr_cleanup(void)
> +void rdma_addr_cleanup(void)
>  {
>         rdma_addr_unregister_client(&self);
>         unregister_netevent_notifier(&nb);
>         destroy_workqueue(addr_wq);
>  }
> -
> -module_init(addr_init);
> -module_exit(addr_cleanup);
> +EXPORT_SYMBOL(rdma_addr_cleanup);
> diff --git a/drivers/infiniband/core/core_priv.h
> b/drivers/infiniband/core/core_priv.h
> index eab32215756b..e0a5187ea98c 100644
> --- a/drivers/infiniband/core/core_priv.h
> +++ b/drivers/infiniband/core/core_priv.h
> @@ -137,4 +137,7 @@ static inline bool rdma_is_upper_dev_rcu(struct
> net_device *dev,
>         return _upper == upper;
>  }
> 
> +int rdma_addr_init(void);
> +void rdma_addr_cleanup(void);
> +
>  #endif /* _CORE_PRIV_H */
> diff --git a/drivers/infiniband/core/device.c
> b/drivers/infiniband/core/device.c
> index 10979844026a..4fdf87a485cc 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -983,10 +983,18 @@ static int __init ib_core_init(void)
>                 goto err_sysfs;
>         }
> 
> +       ret = rdma_addr_init();
> +       if (ret) {
> +               pr_warn("Couldn't init IB address resolution module\n");
> +               goto err_ibnl;
> +       }
> +
>         ib_cache_setup();
> 
>         return 0;
> 
> +err_ibnl:
> +       ibnl_cleanup();
>  err_sysfs:
>         class_unregister(&ib_class);
>  err_comp:
> @@ -999,6 +1007,7 @@ err:
>  static void __exit ib_core_cleanup(void)
>  {
>         ib_cache_cleanup();
> +       rdma_addr_cleanup();
>         ibnl_cleanup();
>         class_unregister(&ib_class);
>         destroy_workqueue(ib_comp_wq);
> 
> 
> 
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 
I'm not sure it will solve the problem. it has been a while since
I've looked into how Linux handles modules so I might be wrong, but:
When we build a module that has to use a symbol from a different module
The symbol is unresolved (in has no address in the .ko file) the moment we load the module,
The kernel searches for the address of the symbol, maps the module into memory
and writes the correct address.

Now in our case, verbs.c is part of ib_core.ko, and it uses rdma_addr_find_l2_eth_by_grh (which is in addr.c, part of ib_addr.ko)
So the moment we try to load ib_core we need to have ib_addr already in memory (because we need the address of rdma_addr_find_l2_eth_by_grh)
modprobe knows about that (because it sees there is a dependency between ib_core and ib_addr) so it tries to load ib_addr.ko first.
ib_addr uses ibnl, so it has  an unresolved symbol, which means ib_core.ko needs to be loaded before it, this is why we
have a cycle. 

After all the theoretical mambo jambo, I had to check it and this is what I got:
Using your code + my ibnl patches, the module_install step fails:
	depmod: ERROR: Found 2 modules in dependency cycles!
	depmod: ERROR: Cycle detected: ib_core -> ib_addr -> ib_core

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 8169 bytes --]

On 05/18/2016 02:43 AM, Mark Bloch wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>> Sent: Tuesday, May 17, 2016 6:49 PM
>> To: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
>> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
>> resolution module into core
>>
>> On 05/17/2016 04:29 AM, Mark Bloch wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>>>> Sent: Monday, May 16, 2016 8:43 PM
>>>> To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
>>>> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
>>>> resolution module into core
>>>> Can you build netlink in and then init the ib_addr module after the
>>>> netlink init is complete?  Wouldn't that resolve the dependency ordering
>>>> issue without changing the module names?
>>> Sorry, but I don't understand what do you mean by this.
>>> If you would like to keep the current module structure (ib_core.ko and
>> ib_addr.ko)
>>> The only way to use ibnl from within addr.c is to move the ibnl initialization
>> to addr.c
>>> and build netlink.c as part of ib_addr.ko, but it seems kind of strange if
>>> ib_addr is responsible to initialize ibnl.
>>
>> Not according to what I was looking at.  In the original patch, you
>> moved addr.c into ib_core and changed addr_init and addr_cleanup from
>> static to normal.  You were then able to control whether the addr code
>> went before or after the netlink code by rearranging the order of init
>> sequences in device.c:ib_core_init().  It is entirely possible that you
>> could leave ib_addr as a module, change addr_init to rdma_addr_init and
>> EXPORT_SYMBOL it, and likewise with the cleanup function, and remove the
>> module_init(addr_init); and module_exit(addr_cleanup); from the addr.c
>> file.  This would then allow you to control when ib_addr was inited by
>> adding a call to rdma_addr_init() in device.c:ib_core_init() just like
>> you had in your patch.  It would solve the problem and give you complete
>> control while being transparent to user space.  This is what I mean:
>>
>> commit ec5394412286e325b83b6c64d026f4d7bab40ccd
>> Author: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> Date:   Tue May 17 11:43:57 2016 -0400
>>
>>     IB/core: change module init ordering
>>
>>     Change ib_addr to be init'ed by ib_core and not by the module load
>>     process.  This gives us precise controle of when the module is
>>     initialized while still preserving the existing module names and load
>>     ordering.  This is to be backward compatible with existing, older
>>     SysV init scripts.  When those older systems are dead and gone, we
>>     can revisit this and change the module names and load orders however
>>     we want.
>>
>>     Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>
>> diff --git a/drivers/infiniband/core/addr.c b/drivers/infiniband/core/addr.c
>> index 337353d86cfa..fda808546e0e 100644
>> --- a/drivers/infiniband/core/addr.c
>> +++ b/drivers/infiniband/core/addr.c
>> @@ -634,7 +634,17 @@ static struct notifier_block nb = {
>>         .notifier_call = netevent_callback
>>  };
>>
>> -static int __init addr_init(void)
>> +/*
>> + * This is a little unorthodox.  We export our init function so that the
>> + * ib_core module can call it at the right time after the other modules
>> + * we depend on have registered.  We do this solely because there are
>> some
>> + * user space init scripts that expect ib_addr to be loaded prior to
>> ib_core
>> + * and due to recent changes, we need the ib netlink code brought up
>> before
>> + * we bring this code up and that code is now in ib_core.  When the older
>> + * SysV init script systems that have these init scripts are dead and gone,
>> + * we can revisit this and possibly just include addr.c in ib_core.
>> + */
>> +int rdma_addr_init(void)
>>  {
>>         addr_wq = create_singlethread_workqueue("ib_addr");
>>         if (!addr_wq)
>> @@ -644,13 +654,12 @@ static int __init addr_init(void)
>>         rdma_addr_register_client(&self);
>>         return 0;
>>  }
>> +EXPORT_SYMBOL(rdma_addr_init);
>>
>> -static void __exit addr_cleanup(void)
>> +void rdma_addr_cleanup(void)
>>  {
>>         rdma_addr_unregister_client(&self);
>>         unregister_netevent_notifier(&nb);
>>         destroy_workqueue(addr_wq);
>>  }
>> -
>> -module_init(addr_init);
>> -module_exit(addr_cleanup);
>> +EXPORT_SYMBOL(rdma_addr_cleanup);
>> diff --git a/drivers/infiniband/core/core_priv.h
>> b/drivers/infiniband/core/core_priv.h
>> index eab32215756b..e0a5187ea98c 100644
>> --- a/drivers/infiniband/core/core_priv.h
>> +++ b/drivers/infiniband/core/core_priv.h
>> @@ -137,4 +137,7 @@ static inline bool rdma_is_upper_dev_rcu(struct
>> net_device *dev,
>>         return _upper == upper;
>>  }
>>
>> +int rdma_addr_init(void);
>> +void rdma_addr_cleanup(void);
>> +
>>  #endif /* _CORE_PRIV_H */
>> diff --git a/drivers/infiniband/core/device.c
>> b/drivers/infiniband/core/device.c
>> index 10979844026a..4fdf87a485cc 100644
>> --- a/drivers/infiniband/core/device.c
>> +++ b/drivers/infiniband/core/device.c
>> @@ -983,10 +983,18 @@ static int __init ib_core_init(void)
>>                 goto err_sysfs;
>>         }
>>
>> +       ret = rdma_addr_init();
>> +       if (ret) {
>> +               pr_warn("Couldn't init IB address resolution module\n");
>> +               goto err_ibnl;
>> +       }
>> +
>>         ib_cache_setup();
>>
>>         return 0;
>>
>> +err_ibnl:
>> +       ibnl_cleanup();
>>  err_sysfs:
>>         class_unregister(&ib_class);
>>  err_comp:
>> @@ -999,6 +1007,7 @@ err:
>>  static void __exit ib_core_cleanup(void)
>>  {
>>         ib_cache_cleanup();
>> +       rdma_addr_cleanup();
>>         ibnl_cleanup();
>>         class_unregister(&ib_class);
>>         destroy_workqueue(ib_comp_wq);
>>
>>
>>
>> --
>> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>               GPG KeyID: 0E572FDD
>>
> I'm not sure it will solve the problem. it has been a while since
> I've looked into how Linux handles modules so I might be wrong, but:
> When we build a module that has to use a symbol from a different module
> The symbol is unresolved (in has no address in the .ko file) the moment we load the module,
> The kernel searches for the address of the symbol, maps the module into memory
> and writes the correct address.
> 
> Now in our case, verbs.c is part of ib_core.ko, and it uses rdma_addr_find_l2_eth_by_grh (which is in addr.c, part of ib_addr.ko)
> So the moment we try to load ib_core we need to have ib_addr already in memory (because we need the address of rdma_addr_find_l2_eth_by_grh)
> modprobe knows about that (because it sees there is a dependency between ib_core and ib_addr) so it tries to load ib_addr.ko first.

Yep.

> ib_addr uses ibnl, so it has  an unresolved symbol,

Yep.

> which means ib_core.ko needs to be loaded before it, this is why we
> have a cycle. 
> 
> After all the theoretical mambo jambo, I had to check it and this is what I got:
> Using your code + my ibnl patches, the module_install step fails:
> 	depmod: ERROR: Found 2 modules in dependency cycles!
> 	depmod: ERROR: Cycle detected: ib_core -> ib_addr -> ib_core

Unless you went back to your original first patch set where ib_netlink
was a separate module, then the load order could be ib_netlink ->
ib_addr -> ib_core and things would work.  Possibly.  Depends on whether
the ib_netlink module has anything in it that references symbols in ib_core.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Wednesday, May 18, 2016 5:17 PM
> To: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
[snip]
> Unless you went back to your original first patch set where ib_netlink
> was a separate module, then the load order could be ib_netlink ->
> ib_addr -> ib_core and things would work.  Possibly.  Depends on whether
> the ib_netlink module has anything in it that references symbols in ib_core.
Yes, the first version did just that and worked, but then there were
comments that we have too many modules and I shouldn't add another one.

V0 adds a new kernel module (ib_netlink.ko ) but leaves ib_core and ib_addr
modules unchanged. I also have a version with Leon's patch
that eliminates ib_addr.ko and doesn't add a new module.

Would you like me to submit  a new version (with Leon's patch) or the review can continue with V0?

> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>               GPG KeyID: 0E572FDD
> 

Mark

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On 05/18/2016 10:26 AM, Mark Bloch wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>> Sent: Wednesday, May 18, 2016 5:17 PM
>> To: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org
>> Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
>> resolution module into core
>>
> [snip]
>> Unless you went back to your original first patch set where ib_netlink
>> was a separate module, then the load order could be ib_netlink ->
>> ib_addr -> ib_core and things would work.  Possibly.  Depends on whether
>> the ib_netlink module has anything in it that references symbols in ib_core.
> Yes, the first version did just that and worked, but then there were
> comments that we have too many modules and I shouldn't add another one.
> 
> V0 adds a new kernel module (ib_netlink.ko ) but leaves ib_core and ib_addr
> modules unchanged. I also have a version with Leon's patch
> that eliminates ib_addr.ko and doesn't add a new module.
> 
> Would you like me to submit  a new version (with Leon's patch) or the review can continue with V0?

One of the things that has come about as a result of this conversation
is that EL6 is the only distro to provide the reload capability.  I have
the ability to make sure it gets updated to account for the module
change, so let's move forward with a version that removes ib_addr and
moves it into the ib_core and also keep the new ib_netlink stuff in
ib_core.  So, fewer modules, not more.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Jason Gunthorpe]

On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
> moves it into the ib_core and also keep the new ib_netlink stuff in
> ib_core.  So, fewer modules, not more.

What about getting rid of ib_sa as well so we can avoid that dynamic
netlink registration patch too?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
>> moves it into the ib_core and also keep the new ib_netlink stuff in
>> ib_core.  So, fewer modules, not more.
> 
> What about getting rid of ib_sa as well so we can avoid that dynamic
> netlink registration patch too?

That's fine too.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Wednesday, May 18, 2016 8:59 PM
> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
> >> moves it into the ib_core and also keep the new ib_netlink stuff in
> >> ib_core.  So, fewer modules, not more.
> >
> > What about getting rid of ib_sa as well so we can avoid that dynamic
> > netlink registration patch too?
> 
> That's fine too.
Integrating ib_sa into ib_core also means to do the same for ib_mad.
If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create the patches.

Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On 05/18/2016 02:28 PM, Mark Bloch wrote:
> 
> 
>> -----Original Message-----
>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>> Sent: Wednesday, May 18, 2016 8:59 PM
>> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
>> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
>> resolution module into core
>>
>> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
>>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
>>>> moves it into the ib_core and also keep the new ib_netlink stuff in
>>>> ib_core.  So, fewer modules, not more.
>>>
>>> What about getting rid of ib_sa as well so we can avoid that dynamic
>>> netlink registration patch too?
>>
>> That's fine too.
> Integrating ib_sa into ib_core also means to do the same for ib_mad.
> If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create the patches.

Ira, Hal?


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Weiny, Ira]

> On 05/18/2016 02:28 PM, Mark Bloch wrote:
> >
> >
> >> -----Original Message-----
> >> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> >> Sent: Wednesday, May 18, 2016 8:59 PM
> >> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> >> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> >> resolution module into core
> >>
> >> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> >>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
> >>>> moves it into the ib_core and also keep the new ib_netlink stuff in
> >>>> ib_core.  So, fewer modules, not more.
> >>>
> >>> What about getting rid of ib_sa as well so we can avoid that dynamic
> >>> netlink registration patch too?
> >>
> >> That's fine too.
> > Integrating ib_sa into ib_core also means to do the same for ib_mad.
> > If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create
> the patches.
> 
> Ira, Hal?
> 

I don't see a problem with that.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote:
> > On 05/18/2016 02:28 PM, Mark Bloch wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > >> Sent: Wednesday, May 18, 2016 8:59 PM
> > >> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > >> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> > >> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > >> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> > >> resolution module into core
> > >>
> > >> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> > >>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
> > >>>> moves it into the ib_core and also keep the new ib_netlink stuff in
> > >>>> ib_core.  So, fewer modules, not more.
> > >>>
> > >>> What about getting rid of ib_sa as well so we can avoid that dynamic
> > >>> netlink registration patch too?
> > >>
> > >> That's fine too.
> > > Integrating ib_sa into ib_core also means to do the same for ib_mad.
> > > If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create
> > the patches.
> > 
> > Ira, Hal?
> > 
> 
> I don't see a problem with that.

Doug,

Will it be acceptable to you if Mark base his patches on this assumption?

Thanks.

> 
> Ira
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1466 bytes --]

On 5/23/2016 4:58 AM, Leon Romanovsky wrote:
> On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote:
>>> On 05/18/2016 02:28 PM, Mark Bloch wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
>>>>> Sent: Wednesday, May 18, 2016 8:59 PM
>>>>> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>>>>> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
>>>>> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
>>>>> resolution module into core
>>>>>
>>>>> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
>>>>>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
>>>>>>> moves it into the ib_core and also keep the new ib_netlink stuff in
>>>>>>> ib_core.  So, fewer modules, not more.
>>>>>>
>>>>>> What about getting rid of ib_sa as well so we can avoid that dynamic
>>>>>> netlink registration patch too?
>>>>>
>>>>> That's fine too.
>>>> Integrating ib_sa into ib_core also means to do the same for ib_mad.
>>>> If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll create
>>> the patches.
>>>
>>> Ira, Hal?
>>>
>>
>> I don't see a problem with that.
> 
> Doug,
> 
> Will it be acceptable to you if Mark base his patches on this assumption?

Yes.



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Mark Bloch]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> Sent: Monday, May 23, 2016 5:06 PM
> To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Weiny, Ira <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> resolution module into core
> 
> On 5/23/2016 4:58 AM, Leon Romanovsky wrote:
> > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote:
> >>> On 05/18/2016 02:28 PM, Mark Bloch wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> >>>>> Sent: Wednesday, May 18, 2016 8:59 PM
> >>>>> To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >>>>> Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; linux-
> >>>>> rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >>>>> Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> >>>>> resolution module into core
> >>>>>
> >>>>> On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> >>>>>> On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford wrote:
> >>>>>>> moves it into the ib_core and also keep the new ib_netlink stuff in
> >>>>>>> ib_core.  So, fewer modules, not more.
> >>>>>>
> >>>>>> What about getting rid of ib_sa as well so we can avoid that dynamic
> >>>>>> netlink registration patch too?
> >>>>>
> >>>>> That's fine too.
> >>>> Integrating ib_sa into ib_core also means to do the same for ib_mad.
> >>>> If you agree with it (ib_sa & ib_mad becoming part of ib_core), I'll
> create
> >>> the patches.
> >>>
> >>> Ira, Hal?
> >>>
> >>
> >> I don't see a problem with that.
> >
> > Doug,
> >
> > Will it be acceptable to you if Mark base his patches on this assumption?
> 
> Yes.
> 

Actually the new series is already posted, 
http://marc.info/?l=linux-rdma&m=146366717406108&w=2
if you can have a look at it'll be great.

Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Knut Omang]

On Mon, 2016-05-23 at 14:09 +0000, Mark Bloch wrote:
> 
> > -----Original Message-----
> > From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > Sent: Monday, May 23, 2016 5:06 PM
> > To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Weiny, Ira <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Jason Gunthorpe
> > <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> > resolution module into core
> > 
> > On 5/23/2016 4:58 AM, Leon Romanovsky wrote:
> > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote:
> > > > > On 05/18/2016 02:28 PM, Mark Bloch wrote:
> > > > > > 
> > > > > > 
> > > > > > > -----Original Message-----
> > > > > > > From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > > > > > > Sent: Wednesday, May 18, 2016 8:59 PM
> > > > > > > To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > > > > Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> > > > > > > linux-
> > > > > > > rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate
> > > > > > > IB address
> > > > > > > resolution module into core
> > > > > > > 
> > > > > > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> > > > > > > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford
> > > > > > > > wrote:
> > > > > > > > > moves it into the ib_core and also keep the new
> > > > > > > > > ib_netlink stuff in
> > > > > > > > > ib_core.  So, fewer modules, not more.
> > > > > > > > 
> > > > > > > > What about getting rid of ib_sa as well so we can avoid
> > > > > > > > that dynamic
> > > > > > > > netlink registration patch too?
> > > > > > > 
> > > > > > > That's fine too.
> > > > > > Integrating ib_sa into ib_core also means to do the same
> > > > > > for ib_mad.
> > > > > > If you agree with it (ib_sa & ib_mad becoming part of
> > > > > > ib_core), I'll
> > create
> > > > > the patches.
> > > > > 
> > > > > Ira, Hal?
> > > > > 

Having just been through a development of a new HCA from scratch, I
found it very useful to have the separation between the core IB
functionality and what I perceive as clients of that core
infrastructure (ib_mad and ib_sa in particular). 

The separation feels natural, and it allows a more piecemeal approach -
a device can be loaded and tested with one WR at a time. Once ib_mad
gets loaded, a lot of "traffic" is created and just about the whole HCA
under development needs to work to support it.

IMHO, if these "clients" gets joined with ib_core, the path for
newcomers will be (even) steeper. I would appreciate if that particular
separation can be kept. I am fine with the other merges, and it might
as well be a good idea to join ib_mad and ib_sa,

Thanks,
Knut Omang
(lead developer of Oracle's new IB HCA, soon to appear here..:-) )

> > > > I don't see a problem with that.
> > > 
> > > Doug,
> > > 
> > > Will it be acceptable to you if Mark base his patches on this
> > > assumption?
> > 
> > Yes.
> > 
> 
> Actually the new series is already posted, 
> http://marc.info/?l=linux-rdma&m=146366717406108&w=2
> if you can have a look at it'll be great.
> 
> Mark
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 4383 bytes --]

On Mon, May 30, 2016 at 07:33:14AM +0200, Knut Omang wrote:
> On Mon, 2016-05-23 at 14:09 +0000, Mark Bloch wrote:
> > 
> > > -----Original Message-----
> > > From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > > Sent: Monday, May 23, 2016 5:06 PM
> > > To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Weiny, Ira <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Jason Gunthorpe
> > > <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > > hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> > > resolution module into core
> > > 
> > > On 5/23/2016 4:58 AM, Leon Romanovsky wrote:
> > > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote:
> > > > > > On 05/18/2016 02:28 PM, Mark Bloch wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > > -----Original Message-----
> > > > > > > > From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > > > > > > > Sent: Wednesday, May 18, 2016 8:59 PM
> > > > > > > > To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > > > > > Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> > > > > > > > linux-
> > > > > > > > rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate
> > > > > > > > IB address
> > > > > > > > resolution module into core
> > > > > > > > 
> > > > > > > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> > > > > > > > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford
> > > > > > > > > wrote:
> > > > > > > > > > moves it into the ib_core and also keep the new
> > > > > > > > > > ib_netlink stuff in
> > > > > > > > > > ib_core.  So, fewer modules, not more.
> > > > > > > > > 
> > > > > > > > > What about getting rid of ib_sa as well so we can avoid
> > > > > > > > > that dynamic
> > > > > > > > > netlink registration patch too?
> > > > > > > > 
> > > > > > > > That's fine too.
> > > > > > > Integrating ib_sa into ib_core also means to do the same
> > > > > > > for ib_mad.
> > > > > > > If you agree with it (ib_sa & ib_mad becoming part of
> > > > > > > ib_core), I'll
> > > create
> > > > > > the patches.
> > > > > > 
> > > > > > Ira, Hal?
> > > > > > 
> 
> Having just been through a development of a new HCA from scratch, I
> found it very useful to have the separation between the core IB
> functionality and what I perceive as clients of that core
> infrastructure (ib_mad and ib_sa in particular). 
> 
> The separation feels natural, and it allows a more piecemeal approach -
> a device can be loaded and tested with one WR at a time. Once ib_mad
> gets loaded, a lot of "traffic" is created and just about the whole HCA
> under development needs to work to support it.

Adding new device to IB is not an easy task and we clearly need to improve in
that area, however these merges are targeted to the end-user of IB stack which
constantly needs to load all these modules.

For the easy development, you can simply revert them in your tree.
Hope it helps.

> 
> IMHO, if these "clients" gets joined with ib_core, the path for
> newcomers will be (even) steeper. I would appreciate if that particular
> separation can be kept. I am fine with the other merges, and it might
> as well be a good idea to join ib_mad and ib_sa,
> 
> Thanks,
> Knut Omang
> (lead developer of Oracle's new IB HCA, soon to appear here..:-) )

Terrific,
We are eager to see it.

> 
> > > > > I don't see a problem with that.
> > > > 
> > > > Doug,
> > > > 
> > > > Will it be acceptable to you if Mark base his patches on this
> > > > assumption?
> > > 
> > > Yes.
> > > 
> > 
> > Actually the new series is already posted, 
> > http://marc.info/?l=linux-rdma&m=146366717406108&w=2
> > if you can have a look at it'll be great.
> > 
> > Mark
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> > in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Knut Omang]

On Mon, 2016-05-30 at 09:33 +0300, Leon Romanovsky wrote:
> On Mon, May 30, 2016 at 07:33:14AM +0200, Knut Omang wrote:
> > On Mon, 2016-05-23 at 14:09 +0000, Mark Bloch wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > > > Sent: Monday, May 23, 2016 5:06 PM
> > > > To: leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Weiny, Ira <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Jason Gunthorpe
> > > > <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > > > hal.rosenstock-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address
> > > > resolution module into core
> > > > 
> > > > On 5/23/2016 4:58 AM, Leon Romanovsky wrote:
> > > > > On Wed, May 18, 2016 at 07:36:03PM +0000, Weiny, Ira wrote:
> > > > > > > On 05/18/2016 02:28 PM, Mark Bloch wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Doug Ledford [mailto:dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org]
> > > > > > > > > Sent: Wednesday, May 18, 2016 8:59 PM
> > > > > > > > > To: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > > > > > > > Cc: Mark Bloch <markb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> > > > > > > > > linux-
> > > > > > > > > rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > > > > > Subject: Re: [PATCH rdma-next V2 5/5] IB/core: Integrate
> > > > > > > > > IB address
> > > > > > > > > resolution module into core
> > > > > > > > > 
> > > > > > > > > On 05/18/2016 01:15 PM, Jason Gunthorpe wrote:
> > > > > > > > > > On Wed, May 18, 2016 at 10:51:02AM -0400, Doug Ledford
> > > > > > > > > > wrote:
> > > > > > > > > > > moves it into the ib_core and also keep the new
> > > > > > > > > > > ib_netlink stuff in
> > > > > > > > > > > ib_core.  So, fewer modules, not more.
> > > > > > > > > > 
> > > > > > > > > > What about getting rid of ib_sa as well so we can avoid
> > > > > > > > > > that dynamic
> > > > > > > > > > netlink registration patch too?
> > > > > > > > > 
> > > > > > > > > That's fine too.
> > > > > > > > Integrating ib_sa into ib_core also means to do the same
> > > > > > > > for ib_mad.
> > > > > > > > If you agree with it (ib_sa & ib_mad becoming part of
> > > > > > > > ib_core), I'll
> > > > create
> > > > > > > the patches.
> > > > > > > 
> > > > > > > Ira, Hal?
> > > > > > > 
> > 
> > Having just been through a development of a new HCA from scratch, I
> > found it very useful to have the separation between the core IB
> > functionality and what I perceive as clients of that core
> > infrastructure (ib_mad and ib_sa in particular). 
> > 
> > The separation feels natural, and it allows a more piecemeal approach -
> > a device can be loaded and tested with one WR at a time. Once ib_mad
> > gets loaded, a lot of "traffic" is created and just about the whole HCA
> > under development needs to work to support it.
> 
> Adding new device to IB is not an easy task and we clearly need to improve in
> that area, however these merges are targeted to the end-user of IB stack which
> constantly needs to load all these modules.

In a well configured system, this should all happen automatically -
this is why we have udev? End users should never have to struggle at
this level. 

> For the easy development, you can simply revert them in your tree.

:-)
Development and debugging needs aside: Keeping separation between
layers is still a good idea IMHO. 

> Hope it helps.
> 
> > 
> > IMHO, if these "clients" gets joined with ib_core, the path for
> > newcomers will be (even) steeper. I would appreciate if that particular
> > separation can be kept. I am fine with the other merges, and it might
> > as well be a good idea to join ib_mad and ib_sa,
> > 
> > Thanks,
> > Knut Omang
> > (lead developer of Oracle's new IB HCA, soon to appear here..:-) )
> 
> Terrific,
> We are eager to see it.

Good things come to those who wait :-) 
Still playing catch-up with the activity here as is evident from my
late comment in this thread..

Knut

> > 
> > > > > > I don't see a problem with that.
> > > > > 
> > > > > Doug,
> > > > > 
> > > > > Will it be acceptable to you if Mark base his patches on this
> > > > > assumption?
> > > > 
> > > > Yes.
> > > > 
> > > 
> > > Actually the new series is already posted, 
> > > http://marc.info/?l=linux-rdma&m=146366717406108&w=2
> > > if you can have a look at it'll be great.
> > > 
> > > Mark
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> > > in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next V2 5/5] IB/core: Integrate IB address resolution module into core

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

On Mon, May 30, 2016 at 01:37:35PM +0200, Knut Omang wrote:
> > > The separation feels natural, and it allows a more piecemeal approach -
> > > a device can be loaded and tested with one WR at a time. Once ib_mad
> > > gets loaded, a lot of "traffic" is created and just about the whole HCA
> > > under development needs to work to support it.
> > 
> > Adding new device to IB is not an easy task and we clearly need to improve in
> > that area, however these merges are targeted to the end-user of IB stack which
> > constantly needs to load all these modules.
> 
> In a well configured system, this should all happen automatically -
> this is why we have udev? End users should never have to struggle at
> this level.
> 
> > For the easy development, you can simply revert them in your tree.
> 
> :-)
> Development and debugging needs aside: Keeping separation between
> layers is still a good idea IMHO.

We tried it for the first version of IB router patches which required
conversion ib_netlink to be a module and found ourselves with bazillion
little modules for every file in IB core.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH rdma-next V2 0/5] Trivial fixes for 4.7

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

On 05/06/2016 03:45 PM, Leon Romanovsky wrote:
> Hi Doug,
> 
> I'm sending to you small collection of trivial fixes to core code.
> 
> It fixes forgotten skb release call, potential overrun in CMA/SA, integration of
> ib_addr.ko into ib_core.ko and one code simplifications which doesn't introduce
> any code change behavior.
> 
> Available in the "topic/fix-core" topic branch of this git repo:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git
> 
> Or for browsing:
> 
> https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/fix-core
> 
> Changes since v2:
>  * Removed iwcm_nl_cb_table size initialization
> 
> Changes since v1:
>  * Added Reviewed-by tags
>  * Revisited titles
>  * Removed RDMA_NL_RDMA_CM_NUM_OPS and RDMA_NL_LS_NUM_OPS
>  * Removed useless enumeration value
>  * Converted ib_addr module to be integral part of ib_core module
> 
> Leon Romanovsky (1):
>   IB/core: Integrate IB address resolution module into core
> 
> Mark Bloch (4):
>   IB/IWPM: Fix a potential skb leak
>   IB/core: Remove unnecessary check in ibnl_rcv_msg
>   IB/core: Fix a potential array overrun in CMA and SA agent
>   IB/SA: Use correct free function
> 
>  drivers/infiniband/core/Makefile    |  6 ++----
>  drivers/infiniband/core/addr.c      | 11 ++---------
>  drivers/infiniband/core/cma.c       |  3 ++-
>  drivers/infiniband/core/device.c    | 11 ++++++++++-
>  drivers/infiniband/core/iwcm.c      |  2 +-
>  drivers/infiniband/core/iwpm_util.c |  1 +
>  drivers/infiniband/core/netlink.c   |  5 ++---
>  drivers/infiniband/core/sa_query.c  |  4 ++--
>  include/rdma/ib_addr.h              |  3 +++
>  9 files changed, 25 insertions(+), 21 deletions(-)
> 

I've taken the first four of these.  I'll respond to the fifth separately.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]