* [PATCH 0/9] IB/core: batch of fix against create/destroy_flow uverbs for v3.12
@ 2013-10-11 17:18 Yann Droneaud
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hi,
As requested by Or Gerlitz, I've rebased my previous fixes [1]
against Matan Barak patch [2]. I've added two patches to fix issues
left or introduced by this patch.
Regarding the previous patchset [1], I've kept the public data
structure improving and renaming patches, but, in order to minimize
the overall changes, I've removed the changes which were modifying
the functions and variables names in the code.
I've removed "code beautifying" and left "hardening" / cleanup changes
for a latter release.
On top of this, I've added the two acknowledged and important patches
from my extended command infrastructure [3]. I've made cleanups in the
changelog regarding the 'email' citations.
It's in pretty good shape, and, if revert or temporarily disabling
are not an option, this can probably go in v3.12.
Regards.
Links:
[1]: http://marc.info/?i=cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1381351016.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2]: https://patchwork.kernel.org/patch/2924341/
[3]: http://marc.info/?i=cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
http://mid.gmane.org/cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Matan Barak (1):
IB/core: clarify overflow/underflow checks on ib_create/destroy_flow
Yann Droneaud (8):
IB/core: Includes flow attribute size in uverbs create_flow
IB/core: Don't include command header size in uverbs create_flow
IB/core: Rename 'flow' structs to match other uverbs structs
IB/core: Makes uverbs flow structure using names more similar to verbs
ones
IB/core: Uses a common header for uverbs flow_specs
IB/core: Remove ib_uverbs_flow_spec structure from userspace
IB/core: extended command: an improved infrastructure for uverbs
commands
IB/core: extended command: move comp_mask to the extended header
drivers/infiniband/core/uverbs.h | 35 +++++++++-
drivers/infiniband/core/uverbs_cmd.c | 101 ++++++++++++++--------------
drivers/infiniband/core/uverbs_main.c | 123 +++++++++++++++++++++++++++-------
drivers/infiniband/hw/mlx4/main.c | 6 +-
include/rdma/ib_verbs.h | 1 +
include/uapi/rdma/ib_user_verbs.h | 95 +++++++++++++++-----------
6 files changed, 240 insertions(+), 121 deletions(-)
--
1.8.3.1
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/9] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
@ 2013-10-11 17:18 ` Yann Droneaud
2013-10-11 17:18 ` [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow Yann Droneaud
` (8 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Matan Barak, linux-rdma-u79uwXL29TY76Z2rM5mHXA
From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
This patch fixes the following issues:
1. Unneeded checks were removed
2. Removed the fixed size out of flow_attr.size and by thus
simplifying the checks.
3. Remove a 32bit hole on 64bit systems with strict alignment
in struct ib_kern_flow_att by adding a reserved field.
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/infiniband/core/uverbs_cmd.c | 27 ++++++++++++---------------
include/uapi/rdma/ib_user_verbs.h | 1 +
2 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index f2b81b9..63c2700 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2654,7 +2654,6 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
void *kern_spec;
void *ib_spec;
int i;
- int kern_attr_size;
if (out_len < sizeof(resp))
return -ENOSPC;
@@ -2669,15 +2668,11 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
!capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
return -EPERM;
- if (cmd.flow_attr.num_of_specs < 0 ||
- cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
+ if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
return -EINVAL;
- kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
- sizeof(struct ib_uverbs_cmd_hdr_ex);
-
- if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
- kern_attr_size < 0 || kern_attr_size >
+ if (cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
+ ib_uverbs_cmd_hdr_ex)) || cmd.flow_attr.size >
(cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
return -EINVAL;
@@ -2688,13 +2683,12 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
- kern_attr_size)) {
+ cmd.flow_attr.size)) {
err = -EFAULT;
goto err_free_attr;
}
} else {
kern_flow_attr = &cmd.flow_attr;
- kern_attr_size = sizeof(cmd.flow_attr);
}
uobj = kmalloc(sizeof(*uobj), GFP_KERNEL);
@@ -2726,19 +2720,22 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
kern_spec = kern_flow_attr + 1;
ib_spec = flow_attr + 1;
- for (i = 0; i < flow_attr->num_of_specs && kern_attr_size > 0; i++) {
+ for (i = 0; i < flow_attr->num_of_specs &&
+ cmd.flow_attr.size > offsetof(struct ib_kern_spec, reserved) &&
+ cmd.flow_attr.size >=
+ ((struct ib_kern_spec *)kern_spec)->size; i++) {
err = kern_spec_to_ib_spec(kern_spec, ib_spec);
if (err)
goto err_free;
flow_attr->size +=
((union ib_flow_spec *) ib_spec)->size;
- kern_attr_size -= ((struct ib_kern_spec *) kern_spec)->size;
+ cmd.flow_attr.size -= ((struct ib_kern_spec *)kern_spec)->size;
kern_spec += ((struct ib_kern_spec *) kern_spec)->size;
ib_spec += ((union ib_flow_spec *) ib_spec)->size;
}
- if (kern_attr_size) {
- pr_warn("create flow failed, %d bytes left from uverb cmd\n",
- kern_attr_size);
+ if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
+ pr_warn("create flow failed, flow %d: %d bytes left from uverb cmd\n",
+ i, cmd.flow_attr.size);
goto err_free;
}
flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 0b233c5..71e994b 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -766,6 +766,7 @@ struct ib_kern_flow_attr {
struct ib_uverbs_create_flow {
__u32 comp_mask;
+ __u32 reserved;
__u64 response;
__u32 qp_handle;
struct ib_kern_flow_attr flow_attr;
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 1/9] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
[not found] ` <a83e37e225cd16e5a556b35dcadc7a1234519c2a.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 3/9] IB/core: Don't include command header " Yann Droneaud
` (7 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In patch "IB/core: clarify overflow/underflow checks on ib_create/destroy_flow",
the meaning of the size field was modified to only represent
the size of the flow_spec appended to the flow_attr structure.
The size of the flow_attr structure must be added when
allocating memory for the whole flow_attr + flow_specs
buffer.
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs_cmd.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 63c2700..3b732f6 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2677,7 +2677,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
return -EINVAL;
if (cmd.flow_attr.num_of_specs) {
- kern_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+ kern_flow_attr = kmalloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
+ GFP_KERNEL);
if (!kern_flow_attr)
return -ENOMEM;
@@ -2705,7 +2706,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
goto err_uobj;
}
- flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+ flow_attr = kmalloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
if (!flow_attr) {
err = -ENOMEM;
goto err_put;
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 1/9] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow Yann Droneaud
2013-10-11 17:18 ` [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
[not found] ` <ca92c22d0aea21d559d57e6152cb313729a11274.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 4/9] IB/core: Rename 'flow' structs to match other uverbs structs Yann Droneaud
` (6 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
Flow spec length don't depend on the size of the command
header: they are part of different layers.
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs_cmd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 3b732f6..1e5f0dd 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2671,8 +2671,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
return -EINVAL;
- if (cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
- ib_uverbs_cmd_hdr_ex)) || cmd.flow_attr.size >
+ if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+ cmd.flow_attr.size >
(cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
return -EINVAL;
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 4/9] IB/core: Rename 'flow' structs to match other uverbs structs
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (2 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 3/9] IB/core: Don't include command header " Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
2013-10-11 17:18 ` [PATCH 5/9] IB/core: Makes uverbs flow structure using names more similar to verbs ones Yann Droneaud
` (5 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
Commit 436f2ad added public data structures to support
receive flow steering. The new structs are not following
the 'uverbs' pattern: they're lacking the common prefix
'ib_uverbs'.
This patch replaces ib_kern prefix by ib_uverbs.
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs_cmd.c | 14 +++++++-------
include/uapi/rdma/ib_user_verbs.h | 36 ++++++++++++++++++------------------
2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 1e5f0dd..20ad9c6 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2599,7 +2599,7 @@ out_put:
return ret ? ret : in_len;
}
-static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
+static int kern_spec_to_ib_spec(struct ib_uverbs_spec *kern_spec,
union ib_flow_spec *ib_spec)
{
ib_spec->type = kern_spec->type;
@@ -2647,7 +2647,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
struct ib_uverbs_create_flow_resp resp;
struct ib_uobject *uobj;
struct ib_flow *flow_id;
- struct ib_kern_flow_attr *kern_flow_attr;
+ struct ib_uverbs_flow_attr *kern_flow_attr;
struct ib_flow_attr *flow_attr;
struct ib_qp *qp;
int err = 0;
@@ -2673,7 +2673,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
cmd.flow_attr.size >
- (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
+ (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_spec)))
return -EINVAL;
if (cmd.flow_attr.num_of_specs) {
@@ -2722,16 +2722,16 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
kern_spec = kern_flow_attr + 1;
ib_spec = flow_attr + 1;
for (i = 0; i < flow_attr->num_of_specs &&
- cmd.flow_attr.size > offsetof(struct ib_kern_spec, reserved) &&
+ cmd.flow_attr.size > offsetof(struct ib_uverbs_spec, reserved) &&
cmd.flow_attr.size >=
- ((struct ib_kern_spec *)kern_spec)->size; i++) {
+ ((struct ib_uverbs_spec *)kern_spec)->size; i++) {
err = kern_spec_to_ib_spec(kern_spec, ib_spec);
if (err)
goto err_free;
flow_attr->size +=
((union ib_flow_spec *) ib_spec)->size;
- cmd.flow_attr.size -= ((struct ib_kern_spec *)kern_spec)->size;
- kern_spec += ((struct ib_kern_spec *) kern_spec)->size;
+ cmd.flow_attr.size -= ((struct ib_uverbs_spec *)kern_spec)->size;
+ kern_spec += ((struct ib_uverbs_spec *) kern_spec)->size;
ib_spec += ((union ib_flow_spec *) ib_spec)->size;
}
if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 71e994b..6a8fd43 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -696,61 +696,61 @@ struct ib_uverbs_detach_mcast {
__u64 driver_data[0];
};
-struct ib_kern_eth_filter {
+struct ib_uverbs_eth_filter {
__u8 dst_mac[6];
__u8 src_mac[6];
__be16 ether_type;
__be16 vlan_tag;
};
-struct ib_kern_spec_eth {
+struct ib_uverbs_spec_eth {
__u32 type;
__u16 size;
__u16 reserved;
- struct ib_kern_eth_filter val;
- struct ib_kern_eth_filter mask;
+ struct ib_uverbs_eth_filter val;
+ struct ib_uverbs_eth_filter mask;
};
-struct ib_kern_ipv4_filter {
+struct ib_uverbs_ipv4_filter {
__be32 src_ip;
__be32 dst_ip;
};
-struct ib_kern_spec_ipv4 {
+struct ib_uverbs_spec_ipv4 {
__u32 type;
__u16 size;
__u16 reserved;
- struct ib_kern_ipv4_filter val;
- struct ib_kern_ipv4_filter mask;
+ struct ib_uverbs_ipv4_filter val;
+ struct ib_uverbs_ipv4_filter mask;
};
-struct ib_kern_tcp_udp_filter {
+struct ib_uverbs_tcp_udp_filter {
__be16 dst_port;
__be16 src_port;
};
-struct ib_kern_spec_tcp_udp {
+struct ib_uverbs_spec_tcp_udp {
__u32 type;
__u16 size;
__u16 reserved;
- struct ib_kern_tcp_udp_filter val;
- struct ib_kern_tcp_udp_filter mask;
+ struct ib_uverbs_tcp_udp_filter val;
+ struct ib_uverbs_tcp_udp_filter mask;
};
-struct ib_kern_spec {
+struct ib_uverbs_spec {
union {
struct {
__u32 type;
__u16 size;
__u16 reserved;
};
- struct ib_kern_spec_eth eth;
- struct ib_kern_spec_ipv4 ipv4;
- struct ib_kern_spec_tcp_udp tcp_udp;
+ struct ib_uverbs_spec_eth eth;
+ struct ib_uverbs_spec_ipv4 ipv4;
+ struct ib_uverbs_spec_tcp_udp tcp_udp;
};
};
-struct ib_kern_flow_attr {
+struct ib_uverbs_flow_attr {
__u32 type;
__u16 size;
__u16 priority;
@@ -769,7 +769,7 @@ struct ib_uverbs_create_flow {
__u32 reserved;
__u64 response;
__u32 qp_handle;
- struct ib_kern_flow_attr flow_attr;
+ struct ib_uverbs_flow_attr flow_attr;
};
struct ib_uverbs_create_flow_resp {
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 5/9] IB/core: Makes uverbs flow structure using names more similar to verbs ones
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (3 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 4/9] IB/core: Rename 'flow' structs to match other uverbs structs Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
2013-10-11 17:18 ` [PATCH 6/9] IB/core: Uses a common header for uverbs flow_specs Yann Droneaud
` (4 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
This patch add "flow" prefix to most of data structure added as part
of commit 436f2ad to keep those names in sync with the data structures
added in commit 319a441.
It's just a matter of translating 'ib_flow' to 'ib_uverbs_flow'.
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs_cmd.c | 12 ++++++------
include/uapi/rdma/ib_user_verbs.h | 32 ++++++++++++++++----------------
2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 20ad9c6..052e07c 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2599,7 +2599,7 @@ out_put:
return ret ? ret : in_len;
}
-static int kern_spec_to_ib_spec(struct ib_uverbs_spec *kern_spec,
+static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
union ib_flow_spec *ib_spec)
{
ib_spec->type = kern_spec->type;
@@ -2673,7 +2673,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
cmd.flow_attr.size >
- (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_spec)))
+ (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
return -EINVAL;
if (cmd.flow_attr.num_of_specs) {
@@ -2722,16 +2722,16 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
kern_spec = kern_flow_attr + 1;
ib_spec = flow_attr + 1;
for (i = 0; i < flow_attr->num_of_specs &&
- cmd.flow_attr.size > offsetof(struct ib_uverbs_spec, reserved) &&
+ cmd.flow_attr.size > offsetof(struct ib_uverbs_flow_spec, reserved) &&
cmd.flow_attr.size >=
- ((struct ib_uverbs_spec *)kern_spec)->size; i++) {
+ ((struct ib_uverbs_flow_spec *)kern_spec)->size; i++) {
err = kern_spec_to_ib_spec(kern_spec, ib_spec);
if (err)
goto err_free;
flow_attr->size +=
((union ib_flow_spec *) ib_spec)->size;
- cmd.flow_attr.size -= ((struct ib_uverbs_spec *)kern_spec)->size;
- kern_spec += ((struct ib_uverbs_spec *) kern_spec)->size;
+ cmd.flow_attr.size -= ((struct ib_uverbs_flow_spec *)kern_spec)->size;
+ kern_spec += ((struct ib_uverbs_flow_spec *) kern_spec)->size;
ib_spec += ((union ib_flow_spec *) ib_spec)->size;
}
if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 6a8fd43..1a097d6 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -696,57 +696,57 @@ struct ib_uverbs_detach_mcast {
__u64 driver_data[0];
};
-struct ib_uverbs_eth_filter {
+struct ib_uverbs_flow_eth_filter {
__u8 dst_mac[6];
__u8 src_mac[6];
__be16 ether_type;
__be16 vlan_tag;
};
-struct ib_uverbs_spec_eth {
+struct ib_uverbs_flow_spec_eth {
__u32 type;
__u16 size;
__u16 reserved;
- struct ib_uverbs_eth_filter val;
- struct ib_uverbs_eth_filter mask;
+ struct ib_uverbs_flow_eth_filter val;
+ struct ib_uverbs_flow_eth_filter mask;
};
-struct ib_uverbs_ipv4_filter {
+struct ib_uverbs_flow_ipv4_filter {
__be32 src_ip;
__be32 dst_ip;
};
-struct ib_uverbs_spec_ipv4 {
+struct ib_uverbs_flow_spec_ipv4 {
__u32 type;
__u16 size;
__u16 reserved;
- struct ib_uverbs_ipv4_filter val;
- struct ib_uverbs_ipv4_filter mask;
+ struct ib_uverbs_flow_ipv4_filter val;
+ struct ib_uverbs_flow_ipv4_filter mask;
};
-struct ib_uverbs_tcp_udp_filter {
+struct ib_uverbs_flow_tcp_udp_filter {
__be16 dst_port;
__be16 src_port;
};
-struct ib_uverbs_spec_tcp_udp {
+struct ib_uverbs_flow_spec_tcp_udp {
__u32 type;
__u16 size;
__u16 reserved;
- struct ib_uverbs_tcp_udp_filter val;
- struct ib_uverbs_tcp_udp_filter mask;
+ struct ib_uverbs_flow_tcp_udp_filter val;
+ struct ib_uverbs_flow_tcp_udp_filter mask;
};
-struct ib_uverbs_spec {
+struct ib_uverbs_flow_spec {
union {
struct {
__u32 type;
__u16 size;
__u16 reserved;
};
- struct ib_uverbs_spec_eth eth;
- struct ib_uverbs_spec_ipv4 ipv4;
- struct ib_uverbs_spec_tcp_udp tcp_udp;
+ struct ib_uverbs_flow_spec_eth eth;
+ struct ib_uverbs_flow_spec_ipv4 ipv4;
+ struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
};
};
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 6/9] IB/core: Uses a common header for uverbs flow_specs
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (4 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 5/9] IB/core: Makes uverbs flow structure using names more similar to verbs ones Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
2013-10-11 17:18 ` [PATCH 7/9] IB/core: Remove ib_uverbs_flow_spec structure from userspace Yann Droneaud
` (3 subsequent siblings)
9 siblings, 0 replies; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
A common header will allows better checking of
flow specs size, while ensuring strict alignment
to 64bits.
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
include/uapi/rdma/ib_user_verbs.h | 53 +++++++++++++++++++++++++++++----------
1 file changed, 40 insertions(+), 13 deletions(-)
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 1a097d6..f7f233b5 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -696,6 +696,14 @@ struct ib_uverbs_detach_mcast {
__u64 driver_data[0];
};
+struct ib_uverbs_flow_spec_hdr {
+ __u32 type;
+ __u16 size;
+ __u16 reserved;
+ /* followed by flow_spec */
+ __u64 flow_spec_data[0];
+};
+
struct ib_uverbs_flow_eth_filter {
__u8 dst_mac[6];
__u8 src_mac[6];
@@ -704,9 +712,14 @@ struct ib_uverbs_flow_eth_filter {
};
struct ib_uverbs_flow_spec_eth {
- __u32 type;
- __u16 size;
- __u16 reserved;
+ union {
+ struct ib_uverbs_flow_spec_hdr hdr;
+ struct {
+ __u32 type;
+ __u16 size;
+ __u16 reserved;
+ };
+ };
struct ib_uverbs_flow_eth_filter val;
struct ib_uverbs_flow_eth_filter mask;
};
@@ -717,9 +730,14 @@ struct ib_uverbs_flow_ipv4_filter {
};
struct ib_uverbs_flow_spec_ipv4 {
- __u32 type;
- __u16 size;
- __u16 reserved;
+ union {
+ struct ib_uverbs_flow_spec_hdr hdr;
+ struct {
+ __u32 type;
+ __u16 size;
+ __u16 reserved;
+ };
+ };
struct ib_uverbs_flow_ipv4_filter val;
struct ib_uverbs_flow_ipv4_filter mask;
};
@@ -730,19 +748,27 @@ struct ib_uverbs_flow_tcp_udp_filter {
};
struct ib_uverbs_flow_spec_tcp_udp {
- __u32 type;
- __u16 size;
- __u16 reserved;
+ union {
+ struct ib_uverbs_flow_spec_hdr hdr;
+ struct {
+ __u32 type;
+ __u16 size;
+ __u16 reserved;
+ };
+ };
struct ib_uverbs_flow_tcp_udp_filter val;
struct ib_uverbs_flow_tcp_udp_filter mask;
};
struct ib_uverbs_flow_spec {
union {
- struct {
- __u32 type;
- __u16 size;
- __u16 reserved;
+ union {
+ struct ib_uverbs_flow_spec_hdr hdr;
+ struct {
+ __u32 type;
+ __u16 size;
+ __u16 reserved;
+ };
};
struct ib_uverbs_flow_spec_eth eth;
struct ib_uverbs_flow_spec_ipv4 ipv4;
@@ -762,6 +788,7 @@ struct ib_uverbs_flow_attr {
* struct ib_flow_spec_xxx
* struct ib_flow_spec_yyy
*/
+ struct ib_uverbs_flow_spec_hdr flow_specs[0];
};
struct ib_uverbs_create_flow {
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 7/9] IB/core: Remove ib_uverbs_flow_spec structure from userspace
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (5 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 6/9] IB/core: Uses a common header for uverbs flow_specs Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
[not found] ` <f8673f302536d503cdeef005a67f4531706ae578.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands Yann Droneaud
` (2 subsequent siblings)
9 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA
The structure holding any types of flow_spec is of no use to userspace.
It would be wrong for userspace to do:
struct ib_uverbs_flow_spec flow_spec;
flow_spec.type = IB_FLOW_SPEC_TCP;
flow_spec.size = sizeof(flow_spec);
Instead, userspace should use the dedicated flow_spec structure for
- Ethernet : struct ib_uverbs_flow_spec_eth,
- IPv4 : struct ib_uverbs_flow_spec_ipv4,
- TCP/UDP : struct ib_uverbs_flow_spec_tcp_udp.
In other words, struct ib_uverbs_flow_spec is a "virtual"
data structure that can only be use by the kernel as an alias
to the other.
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs.h | 16 ++++++++++++++++
include/uapi/rdma/ib_user_verbs.h | 16 ----------------
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index d040b87..4ae0307 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -178,6 +178,22 @@ void ib_uverbs_event_handler(struct ib_event_handler *handler,
struct ib_event *event);
void ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd);
+struct ib_uverbs_flow_spec {
+ union {
+ union {
+ struct ib_uverbs_flow_spec_hdr hdr;
+ struct {
+ __u32 type;
+ __u16 size;
+ __u16 reserved;
+ };
+ };
+ struct ib_uverbs_flow_spec_eth eth;
+ struct ib_uverbs_flow_spec_ipv4 ipv4;
+ struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
+ };
+};
+
#define IB_UVERBS_DECLARE_CMD(name) \
ssize_t ib_uverbs_##name(struct ib_uverbs_file *file, \
const char __user *buf, int in_len, \
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index f7f233b5..93577fc 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -760,22 +760,6 @@ struct ib_uverbs_flow_spec_tcp_udp {
struct ib_uverbs_flow_tcp_udp_filter mask;
};
-struct ib_uverbs_flow_spec {
- union {
- union {
- struct ib_uverbs_flow_spec_hdr hdr;
- struct {
- __u32 type;
- __u16 size;
- __u16 reserved;
- };
- };
- struct ib_uverbs_flow_spec_eth eth;
- struct ib_uverbs_flow_spec_ipv4 ipv4;
- struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
- };
-};
-
struct ib_uverbs_flow_attr {
__u32 type;
__u16 size;
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (6 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 7/9] IB/core: Remove ib_uverbs_flow_spec structure from userspace Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
[not found] ` <0b84e20b204eb0fc67db9ca9106d6bf753a12ae3.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 9/9] IB/core: extended command: move comp_mask to the extended header Yann Droneaud
2013-10-14 15:36 ` [PATCH 0/9] IB/core: batch of fix against create/destroy_flow uverbs for v3.12 Or Gerlitz
9 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Igor Ivanov,
Matan Barak
Commit 400dbc9 added an infrastructure for extensible uverbs
commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow()
functions using this new infrastructure.
According to the commit 400dbc9, the purpose of this infrastructure is
to support passing around provider (eg. hardware) specific buffers
when userspace issue commands to the kernel, so that it would be possible
to extend uverbs (eg. core) buffers independently from the provider buffers.
But the new kernel command function prototypes were not modified
to take advantage of this extension. This issue was exposed by
Roland Dreier in a previous review[1].
So the following patch is an attempt to a revised extensible command
infrastructure.
This improved extensible command infrastructure distinguish between
core (eg. legacy)'s command/response buffers from provider (eg. hardware)'s
command/response buffers: each extended command implementing function
is given a struct ib_udata to hold core (eg. uverbs) input and output buffers,
and another struct ib_udata to hold the hw (eg. provider) input and output
buffers.
Having those buffers identified separately make it easier to increase one
buffer to support extension without having to add some code to guess
the exact size of each command/response parts: This should make the extended
functions more reliable.
Additionally, instead of relying on command identifier being greater
than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure
rely on unused bits in command field: on the 32 bits provided by
command field, only 6 bits are really needed to encode the identifier
of commands currently supported by the kernel. (Even using only 6 bits
leaves room for about 23 new commands).
So this patch makes use of some high order bits in command field
to store flags, leaving enough room for more command identifiers
than one will ever need (eg. 256).
The new flags are used to specify if the command should be processed
as an extended one or a legacy one. While designing the new command format,
care was taken to make usage of flags itself extensible.
Using high order bits of the commands field ensure
that newer libibverbs on older kernel will properly fail
when trying to call extended commands. On the other hand,
older libibverbs on newer kernel will never be able to issue
calls to extended commands.
The extended command header includes the optional response
pointer so that output buffer length and output buffer pointer
are located together in the command, allowing proper parameters
checking. This should make implementing functions easier and safer.
Additionally the extended header ensure 64bits alignment,
while making all sizes multiple of 8 bytes, extending
the maximum buffer size:
legacy extended
Maximum command buffer: 256KBytes 1024KBytes (512KBytes + 512KBytes)
Maximum response buffer: 256KBytes 1024KBytes (512KBytes + 512KBytes)
For the purpose of doing proper buffer size accounting, the headers size
are no more taken in account in "in_words".
One of the odds of the current extensible infrastructure, reading twice
the "legacy" command header, is fixed by removing the "legacy" command header
from the extended command header: they are processed as two different parts
of the command: memory is read once and information are not duplicated:
it's making clear that's an extended command scheme and not a different
command scheme.
The proposed scheme will format input (command) and output (response)
buffers this way:
- command:
legacy header +
extended header +
command data (core + hw):
+----------------------------------------+
| flags | 00 00 | command |
| in_words | out_words |
+----------------------------------------+
| response |
| response |
| provider_in_words | provider_out_words |
| padding |
+----------------------------------------+
| |
. <uverbs input> .
. (in_words * 8) .
| |
+----------------------------------------+
| |
. <provider input> .
. (provider_in_words * 8) .
| |
+----------------------------------------+
- response, if present:
+----------------------------------------+
| |
. <uverbs output space> .
. (out_words * 8) .
| |
+----------------------------------------+
| |
. <provider output space> .
. (provider_out_words * 8) .
| |
+----------------------------------------+
The overall design is to ensure that the extensible
infrastructure is itself extensible while begin more
reliable with more input and bound checking.
[1]:
http://marc.info/?i=CAL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs.h | 18 ++++-
drivers/infiniband/core/uverbs_cmd.c | 56 ++++++++--------
drivers/infiniband/core/uverbs_main.c | 121 ++++++++++++++++++++++++++--------
drivers/infiniband/hw/mlx4/main.c | 6 +-
include/rdma/ib_verbs.h | 1 +
include/uapi/rdma/ib_user_verbs.h | 21 +++---
6 files changed, 154 insertions(+), 69 deletions(-)
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 4ae0307..bdc842e 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -47,6 +47,14 @@
#include <rdma/ib_umem.h>
#include <rdma/ib_user_verbs.h>
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
+ do { \
+ (udata)->inbuf = (void __user *) (ibuf); \
+ (udata)->outbuf = (void __user *) (obuf); \
+ (udata)->inlen = (ilen); \
+ (udata)->outlen = (olen); \
+ } while (0)
+
/*
* Our lifetime rules for these structs are the following:
*
@@ -233,7 +241,13 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
IB_UVERBS_DECLARE_CMD(create_xsrq);
IB_UVERBS_DECLARE_CMD(open_xrcd);
IB_UVERBS_DECLARE_CMD(close_xrcd);
-IB_UVERBS_DECLARE_CMD(create_flow);
-IB_UVERBS_DECLARE_CMD(destroy_flow);
+
+#define IB_UVERBS_DECLARE_EX_CMD(name) \
+ int ib_uverbs_ex_##name(struct ib_uverbs_file *file, \
+ struct ib_udata *ucore, \
+ struct ib_udata *uhw)
+
+IB_UVERBS_DECLARE_EX_CMD(create_flow);
+IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
#endif /* UVERBS_H */
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 052e07c..e35877d 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -56,14 +56,6 @@ static struct uverbs_lock_class srq_lock_class = { .name = "SRQ-uobj" };
static struct uverbs_lock_class xrcd_lock_class = { .name = "XRCD-uobj" };
static struct uverbs_lock_class rule_lock_class = { .name = "RULE-uobj" };
-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
- do { \
- (udata)->inbuf = (void __user *) (ibuf); \
- (udata)->outbuf = (void __user *) (obuf); \
- (udata)->inlen = (ilen); \
- (udata)->outlen = (olen); \
- } while (0)
-
/*
* The ib_uobject locking scheme is as follows:
*
@@ -2639,9 +2631,9 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
return 0;
}
-ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
- const char __user *buf, int in_len,
- int out_len)
+int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
+ struct ib_udata *ucore,
+ struct ib_udata *uhw)
{
struct ib_uverbs_create_flow cmd;
struct ib_uverbs_create_flow_resp resp;
@@ -2655,11 +2647,15 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
void *ib_spec;
int i;
- if (out_len < sizeof(resp))
+ if (ucore->outlen < sizeof(resp))
return -ENOSPC;
- if (copy_from_user(&cmd, buf, sizeof(cmd)))
- return -EFAULT;
+ err = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
+ if (err)
+ return err;
+
+ ucore->inbuf += sizeof(cmd);
+ ucore->inlen -= sizeof(cmd);
if (cmd.comp_mask)
return -EINVAL;
@@ -2671,7 +2667,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
return -EINVAL;
- if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+ if (cmd.flow_attr.size > ucore->inlen ||
cmd.flow_attr.size >
(cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
return -EINVAL;
@@ -2683,11 +2679,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
return -ENOMEM;
memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
- if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
- cmd.flow_attr.size)) {
- err = -EFAULT;
+ err = ib_copy_from_udata(kern_flow_attr + 1, ucore,
+ cmd.flow_attr.size);
+ if (err)
goto err_free_attr;
- }
} else {
kern_flow_attr = &cmd.flow_attr;
}
@@ -2755,11 +2750,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
memset(&resp, 0, sizeof(resp));
resp.flow_handle = uobj->id;
- if (copy_to_user((void __user *)(unsigned long) cmd.response,
- &resp, sizeof(resp))) {
- err = -EFAULT;
+ err = ib_copy_to_udata(ucore,
+ &resp, sizeof(resp));
+ if (err)
goto err_copy;
- }
put_qp_read(qp);
mutex_lock(&file->mutex);
@@ -2772,7 +2766,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
kfree(flow_attr);
if (cmd.flow_attr.num_of_specs)
kfree(kern_flow_attr);
- return in_len;
+ return 0;
err_copy:
idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
destroy_flow:
@@ -2789,16 +2783,18 @@ err_free_attr:
return err;
}
-ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
- const char __user *buf, int in_len,
- int out_len) {
+int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
+ struct ib_udata *ucore,
+ struct ib_udata *uhw)
+{
struct ib_uverbs_destroy_flow cmd;
struct ib_flow *flow_id;
struct ib_uobject *uobj;
int ret;
- if (copy_from_user(&cmd, buf, sizeof(cmd)))
- return -EFAULT;
+ ret = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
+ if (ret)
+ return ret;
uobj = idr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
file->ucontext);
@@ -2820,7 +2816,7 @@ ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
put_uobj(uobj);
- return ret ? ret : in_len;
+ return ret ? ret : 0;
}
static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 75ad86c..4f00654 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -115,8 +115,13 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
[IB_USER_VERBS_CMD_CLOSE_XRCD] = ib_uverbs_close_xrcd,
[IB_USER_VERBS_CMD_CREATE_XSRQ] = ib_uverbs_create_xsrq,
[IB_USER_VERBS_CMD_OPEN_QP] = ib_uverbs_open_qp,
- [IB_USER_VERBS_CMD_CREATE_FLOW] = ib_uverbs_create_flow,
- [IB_USER_VERBS_CMD_DESTROY_FLOW] = ib_uverbs_destroy_flow
+};
+
+static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
+ struct ib_udata *ucore,
+ struct ib_udata *uhw) = {
+ [IB_USER_VERBS_EX_CMD_CREATE_FLOW] = ib_uverbs_ex_create_flow,
+ [IB_USER_VERBS_EX_CMD_DESTROY_FLOW] = ib_uverbs_ex_destroy_flow
};
static void ib_uverbs_add_one(struct ib_device *device);
@@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
{
struct ib_uverbs_file *file = filp->private_data;
struct ib_uverbs_cmd_hdr hdr;
+ __u32 flags;
if (count < sizeof hdr)
return -EINVAL;
@@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
if (copy_from_user(&hdr, buf, sizeof hdr))
return -EFAULT;
- if (hdr.command >= ARRAY_SIZE(uverbs_cmd_table) ||
- !uverbs_cmd_table[hdr.command])
- return -EINVAL;
+ flags = (hdr.command &
+ IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
- if (!file->ucontext &&
- hdr.command != IB_USER_VERBS_CMD_GET_CONTEXT)
- return -EINVAL;
+ if (!flags) {
+ __u32 command;
- if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
- return -ENOSYS;
+ if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+ IB_USER_VERBS_CMD_COMMAND_MASK))
+ return -EINVAL;
- if (hdr.command >= IB_USER_VERBS_CMD_THRESHOLD) {
- struct ib_uverbs_cmd_hdr_ex hdr_ex;
+ command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
- if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
- return -EFAULT;
+ if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
+ !uverbs_cmd_table[command])
+ return -EINVAL;
- if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4) != count)
+ if (!file->ucontext &&
+ command != IB_USER_VERBS_CMD_GET_CONTEXT)
return -EINVAL;
- return uverbs_cmd_table[hdr.command](file,
- buf + sizeof(hdr_ex),
- (hdr_ex.in_words +
- hdr_ex.provider_in_words) * 4,
- (hdr_ex.out_words +
- hdr_ex.provider_out_words) * 4);
- } else {
+ if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command)))
+ return -ENOSYS;
+
if (hdr.in_words * 4 != count)
return -EINVAL;
- return uverbs_cmd_table[hdr.command](file,
- buf + sizeof(hdr),
- hdr.in_words * 4,
- hdr.out_words * 4);
+ return uverbs_cmd_table[command](file,
+ buf + sizeof(hdr),
+ hdr.in_words * 4,
+ hdr.out_words * 4);
+
+ } else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
+ __u32 command;
+
+ struct ib_uverbs_ex_cmd_hdr ex_hdr;
+ struct ib_udata ucore;
+ struct ib_udata uhw;
+ int err;
+
+ if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+ IB_USER_VERBS_CMD_COMMAND_MASK))
+ return -EINVAL;
+
+ command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
+ if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
+ !uverbs_ex_cmd_table[command])
+ return -ENOSYS;
+
+ if (!file->ucontext)
+ return -EINVAL;
+
+ if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command)))
+ return -ENOSYS;
+
+ if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+ return -EINVAL;
+
+ if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr)))
+ return -EFAULT;
+
+ count -= sizeof(hdr) + sizeof(ex_hdr);
+ buf += sizeof(hdr) + sizeof(ex_hdr);
+
+ if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
+ return -EINVAL;
+
+ if (ex_hdr.response) {
+ if (!hdr.out_words && !ex_hdr.provider_out_words)
+ return -EINVAL;
+ } else {
+ if (hdr.out_words || ex_hdr.provider_out_words)
+ return -EINVAL;
+ }
+
+ INIT_UDATA(&ucore,
+ (hdr.in_words) ? buf : 0,
+ (unsigned long)ex_hdr.response,
+ hdr.in_words * 8,
+ hdr.out_words * 8);
+
+ INIT_UDATA(&uhw,
+ (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
+ (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0,
+ ex_hdr.provider_in_words * 8,
+ ex_hdr.provider_out_words * 8);
+
+ err = uverbs_ex_cmd_table[command](file,
+ &ucore,
+ &uhw);
+
+ if (err)
+ return err;
+
+ return count;
}
+
+ return -ENOSYS;
}
static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index d6c5a73..1aad9b3 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1691,9 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
ibdev->ib_dev.create_flow = mlx4_ib_create_flow;
ibdev->ib_dev.destroy_flow = mlx4_ib_destroy_flow;
- ibdev->ib_dev.uverbs_cmd_mask |=
- (1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |
- (1ull << IB_USER_VERBS_CMD_DESTROY_FLOW);
+ ibdev->ib_dev.uverbs_ex_cmd_mask |=
+ (1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |
+ (1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
}
mlx4_ib_alloc_eqs(dev, ibdev);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index e393171..a06fc12 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1436,6 +1436,7 @@ struct ib_device {
int uverbs_abi_ver;
u64 uverbs_cmd_mask;
+ u64 uverbs_ex_cmd_mask;
char node_desc[64];
__be64 node_guid;
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 93577fc..cbfdd4c 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -87,8 +87,11 @@ enum {
IB_USER_VERBS_CMD_CLOSE_XRCD,
IB_USER_VERBS_CMD_CREATE_XSRQ,
IB_USER_VERBS_CMD_OPEN_QP,
- IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
- IB_USER_VERBS_CMD_DESTROY_FLOW
+};
+
+enum {
+ IB_USER_VERBS_EX_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
+ IB_USER_VERBS_EX_CMD_DESTROY_FLOW
};
/*
@@ -120,16 +123,20 @@ struct ib_uverbs_comp_event_desc {
* the rest of the command struct based on these value.
*/
+#define IB_USER_VERBS_CMD_COMMAND_MASK 0xff
+#define IB_USER_VERBS_CMD_FLAGS_MASK 0xff000000u
+#define IB_USER_VERBS_CMD_FLAGS_SHIFT 24
+
+#define IB_USER_VERBS_CMD_FLAG_EXTENDED 0x80
+
struct ib_uverbs_cmd_hdr {
__u32 command;
__u16 in_words;
__u16 out_words;
};
-struct ib_uverbs_cmd_hdr_ex {
- __u32 command;
- __u16 in_words;
- __u16 out_words;
+struct ib_uverbs_ex_cmd_hdr {
+ __u64 response;
__u16 provider_in_words;
__u16 provider_out_words;
__u32 cmd_hdr_reserved;
@@ -777,8 +784,6 @@ struct ib_uverbs_flow_attr {
struct ib_uverbs_create_flow {
__u32 comp_mask;
- __u32 reserved;
- __u64 response;
__u32 qp_handle;
struct ib_uverbs_flow_attr flow_attr;
};
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 9/9] IB/core: extended command: move comp_mask to the extended header
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (7 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands Yann Droneaud
@ 2013-10-11 17:18 ` Yann Droneaud
[not found] ` <40175eda10d670d098204da6aa4c327a0171ae5f.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 15:36 ` [PATCH 0/9] IB/core: batch of fix against create/destroy_flow uverbs for v3.12 Or Gerlitz
9 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-11 17:18 UTC (permalink / raw)
To: Roland Dreier, Roland Dreier, Or Gerlitz
Cc: Yann Droneaud, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Igor Ivanov,
Matan Barak
The unused field in the extended header is a perfect candidate
to hold the command "comp_mask" (eg. bit field used to handle
compatibility). This was suggested by Roland Dreier in a previous
review[1].
So this patch move comp_mask from create_flow/destroy_flow commands
to the extended command header. Then comp_mask is passed as part
of function parameters.
[1]:
http://marc.info/?i=CAL1RGDXJtrc849M6_XNZT5xO1+ybKtLWGq6yg6LhoSsKpsmkYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
drivers/infiniband/core/uverbs.h | 3 ++-
drivers/infiniband/core/uverbs_cmd.c | 15 ++++++++++-----
drivers/infiniband/core/uverbs_main.c | 6 ++++--
include/uapi/rdma/ib_user_verbs.h | 6 +++---
4 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index bdc842e..a12b516 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -245,7 +245,8 @@ IB_UVERBS_DECLARE_CMD(close_xrcd);
#define IB_UVERBS_DECLARE_EX_CMD(name) \
int ib_uverbs_ex_##name(struct ib_uverbs_file *file, \
struct ib_udata *ucore, \
- struct ib_udata *uhw)
+ struct ib_udata *uhw, \
+ __u32 comp_mask)
IB_UVERBS_DECLARE_EX_CMD(create_flow);
IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index e35877d..a8ecb3a 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2633,7 +2633,8 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
struct ib_udata *ucore,
- struct ib_udata *uhw)
+ struct ib_udata *uhw,
+ __u32 comp_mask)
{
struct ib_uverbs_create_flow cmd;
struct ib_uverbs_create_flow_resp resp;
@@ -2647,6 +2648,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
void *ib_spec;
int i;
+ if (comp_mask)
+ return -EINVAL;
+
if (ucore->outlen < sizeof(resp))
return -ENOSPC;
@@ -2657,9 +2661,6 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
ucore->inbuf += sizeof(cmd);
ucore->inlen -= sizeof(cmd);
- if (cmd.comp_mask)
- return -EINVAL;
-
if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
!capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
return -EPERM;
@@ -2785,13 +2786,17 @@ err_free_attr:
int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
struct ib_udata *ucore,
- struct ib_udata *uhw)
+ struct ib_udata *uhw,
+ __u32 comp_mask)
{
struct ib_uverbs_destroy_flow cmd;
struct ib_flow *flow_id;
struct ib_uobject *uobj;
int ret;
+ if (comp_mask)
+ return -EINVAL;
+
ret = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
if (ret)
return ret;
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 4f00654..a9687dd 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -119,7 +119,8 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
struct ib_udata *ucore,
- struct ib_udata *uhw) = {
+ struct ib_udata *uhw,
+ __u32 comp_mask) = {
[IB_USER_VERBS_EX_CMD_CREATE_FLOW] = ib_uverbs_ex_create_flow,
[IB_USER_VERBS_EX_CMD_DESTROY_FLOW] = ib_uverbs_ex_destroy_flow
};
@@ -689,7 +690,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
err = uverbs_ex_cmd_table[command](file,
&ucore,
- &uhw);
+ &uhw,
+ ex_hdr.comp_mask);
if (err)
return err;
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index cbfdd4c..095a2bd 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -139,7 +139,7 @@ struct ib_uverbs_ex_cmd_hdr {
__u64 response;
__u16 provider_in_words;
__u16 provider_out_words;
- __u32 cmd_hdr_reserved;
+ __u32 comp_mask;
};
struct ib_uverbs_get_context {
@@ -783,8 +783,8 @@ struct ib_uverbs_flow_attr {
};
struct ib_uverbs_create_flow {
- __u32 comp_mask;
__u32 qp_handle;
+ __u32 reserved;
struct ib_uverbs_flow_attr flow_attr;
};
@@ -794,8 +794,8 @@ struct ib_uverbs_create_flow_resp {
};
struct ib_uverbs_destroy_flow {
- __u32 comp_mask;
__u32 flow_handle;
+ __u32 reserved;
};
struct ib_uverbs_create_srq {
--
1.8.3.1
--
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
^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow
[not found] ` <a83e37e225cd16e5a556b35dcadc7a1234519c2a.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
@ 2013-10-14 11:01 ` Or Gerlitz
[not found] ` <525BCF21.3090706-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-14 15:13 ` Matan Barak
1 sibling, 1 reply; 31+ messages in thread
From: Or Gerlitz @ 2013-10-14 11:01 UTC (permalink / raw)
To: Yann Droneaud, Matan Barak
Cc: Roland Dreier, Roland Dreier, linux-rdma-u79uwXL29TY76Z2rM5mHXA
On 11/10/2013 20:18, Yann Droneaud wrote:
> In patch "IB/core: clarify overflow/underflow checks on ib_create/destroy_flow",
> the meaning of the size field was modified to only represent
> the size of the flow_spec appended to the flow_attr structure.
>
> The size of the flow_attr structure must be added when
> allocating memory for the whole flow_attr + flow_specs
> buffer.
wait, patch #2 fixes a problem introduced in patch #1? if this is the
case, why not change patch #1?
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
> drivers/infiniband/core/uverbs_cmd.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 63c2700..3b732f6 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2677,7 +2677,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> return -EINVAL;
>
> if (cmd.flow_attr.num_of_specs) {
> - kern_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
> + kern_flow_attr = kmalloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
> + GFP_KERNEL);
> if (!kern_flow_attr)
> return -ENOMEM;
>
> @@ -2705,7 +2706,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> goto err_uobj;
> }
>
> - flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
> + flow_attr = kmalloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
> if (!flow_attr) {
> err = -ENOMEM;
> goto err_put;
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow
[not found] ` <525BCF21.3090706-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-10-14 12:22 ` Yann Droneaud
0 siblings, 0 replies; 31+ messages in thread
From: Yann Droneaud @ 2013-10-14 12:22 UTC (permalink / raw)
To: Or Gerlitz
Cc: Matan Barak, Roland Dreier, Roland Dreier,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hi,
Le 14.10.2013 13:01, Or Gerlitz a écrit :
> On 11/10/2013 20:18, Yann Droneaud wrote:
>> In patch "IB/core: clarify overflow/underflow checks on
>> ib_create/destroy_flow",
>> the meaning of the size field was modified to only represent
>> the size of the flow_spec appended to the flow_attr structure.
>>
>> The size of the flow_attr structure must be added when
>> allocating memory for the whole flow_attr + flow_specs
>> buffer.
>
> wait, patch #2 fixes a problem introduced in patch #1? if this is the
> case, why not change patch #1?
You specifically asked me to rebase the patchset against Matan's patch
instead of addressing the issues with my own.
While I do think my patchset was better, I complied and used the
aforementioned
patch, but had to add fixes which were implicit with my patchset.
Regards.
--
Yann Droneaud
OPTEYA
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow
[not found] ` <a83e37e225cd16e5a556b35dcadc7a1234519c2a.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 11:01 ` Or Gerlitz
@ 2013-10-14 15:13 ` Matan Barak
1 sibling, 0 replies; 31+ messages in thread
From: Matan Barak @ 2013-10-14 15:13 UTC (permalink / raw)
To: Yann Droneaud, Roland Dreier, Roland Dreier, Or Gerlitz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 11/10/2013 8:19 PM, Yann Droneaud wrote:
> In patch "IB/core: clarify overflow/underflow checks on ib_create/destroy_flow",
> the meaning of the size field was modified to only represent
> the size of the flow_spec appended to the flow_attr structure.
>
> The size of the flow_attr structure must be added when
> allocating memory for the whole flow_attr + flow_specs
> buffer.
>
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
> drivers/infiniband/core/uverbs_cmd.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 63c2700..3b732f6 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2677,7 +2677,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> return -EINVAL;
>
> if (cmd.flow_attr.num_of_specs) {
> - kern_flow_attr =malloc(cmd.flow_attr.size, GFP_KERNEL);
> + kern_flow_attr =malloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
> + GFP_KERNEL);
> if (!kern_flow_attr)
> return -ENOMEM;
>
> @@ -2705,7 +2706,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> goto err_uobj;
> }
>
> - flow_attr =malloc(cmd.flow_attr.size, GFP_KERNEL);
> + flow_attr =malloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
> if (!flow_attr) {
> err =ENOMEM;
> goto err_put;
> --
> 1.8.3.1
>
Hi,
Since this patch mainly fixes PATCH 1, I see no point on sending them as
2 different patches. Squashing them seems like a better idea.
Regards.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <ca92c22d0aea21d559d57e6152cb313729a11274.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
@ 2013-10-14 15:21 ` Matan Barak
[not found] ` <525C0BF2.2060201-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2013-10-14 15:21 UTC (permalink / raw)
To: Yann Droneaud, Roland Dreier, Roland Dreier, Or Gerlitz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 11/10/2013 8:19 PM, Yann Droneaud wrote:
> Flow spec length don't depend on the size of the command
> header: they are part of different layers.
>
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
> drivers/infiniband/core/uverbs_cmd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 3b732f6..1e5f0dd 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2671,8 +2671,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
> return -EINVAL;
>
> - if (cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
> - ib_uverbs_cmd_hdr_ex)) || cmd.flow_attr.size >
> + if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
> + cmd.flow_attr.size >
> (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
> return -EINVAL;
>
> --
> 1.8.3.1
>
Hi,
I agree that it was better if the command size we pass to the uverb
didn't include the header size, but since the current implementation of
ib_uverbs_write and its user-space counterpart passes the command size
including the header - I think this fix is incorrect. I think it'll
actually break the current implementation.
Furthermore, patch 8 overwrites this anyway, so I guess we could reduce
this patchset size by removing it.
Regards,
Matan.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 7/9] IB/core: Remove ib_uverbs_flow_spec structure from userspace
[not found] ` <f8673f302536d503cdeef005a67f4531706ae578.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
@ 2013-10-14 15:26 ` Matan Barak
0 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2013-10-14 15:26 UTC (permalink / raw)
To: Yann Droneaud, Roland Dreier, Roland Dreier, Or Gerlitz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 11/10/2013 8:56 PM, Yann Droneaud wrote:
> The structure holding any types of flow_spec is of no use to userspace.
> It would be wrong for userspace to do:
>
> struct ib_uverbs_flow_spec flow_spec;
>
> flow_spec.type =B_FLOW_SPEC_TCP;
> flow_spec.size =izeof(flow_spec);
>
> Instead, userspace should use the dedicated flow_spec structure for
> - Ethernet : struct ib_uverbs_flow_spec_eth,
> - IPv4 : struct ib_uverbs_flow_spec_ipv4,
> - TCP/UDP : struct ib_uverbs_flow_spec_tcp_udp.
>
> In other words, struct ib_uverbs_flow_spec is a "virtual"
> data structure that can only be use by the kernel as an alias
> to the other.
>
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
> drivers/infiniband/core/uverbs.h | 16 ++++++++++++++++
> include/uapi/rdma/ib_user_verbs.h | 16 ----------------
> 2 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index d040b87..4ae0307 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -178,6 +178,22 @@ void ib_uverbs_event_handler(struct ib_event_handler *handler,
> struct ib_event *event);
> void ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd);
>
> +struct ib_uverbs_flow_spec {
> + union {
> + union {
> + struct ib_uverbs_flow_spec_hdr hdr;
> + struct {
> + __u32 type;
> + __u16 size;
> + __u16 reserved;
> + };
> + };
> + struct ib_uverbs_flow_spec_eth eth;
> + struct ib_uverbs_flow_spec_ipv4 ipv4;
> + struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
> + };
> +};
> +
> #define IB_UVERBS_DECLARE_CMD(name) \
> ssize_t ib_uverbs_##name(struct ib_uverbs_file *file, \
> const char __user *buf, int in_len, \
> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
> index f7f233b5..93577fc 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -760,22 +760,6 @@ struct ib_uverbs_flow_spec_tcp_udp {
> struct ib_uverbs_flow_tcp_udp_filter mask;
> };
>
> -struct ib_uverbs_flow_spec {
> - union {
> - union {
> - struct ib_uverbs_flow_spec_hdr hdr;
> - struct {
> - __u32 type;
> - __u16 size;
> - __u16 reserved;
> - };
> - };
> - struct ib_uverbs_flow_spec_eth eth;
> - struct ib_uverbs_flow_spec_ipv4 ipv4;
> - struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
> - };
> -};
> -
> struct ib_uverbs_flow_attr {
> __u32 type;
> __u16 size;
> --
> 1.8.3.1
>
Hi,
Nice catch - exposing struct ib_uverbs_flow_spec to userspace imposes
another risk. Adding a spec that is larger than all previous specs will
result in a larger ib_uverbs_flow_spec structure. This in turn could
lead to ABI breaks - something we definitely don't want. The ABI
shouldn't include a "do-it-all" spec.
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/9] IB/core: batch of fix against create/destroy_flow uverbs for v3.12
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
` (8 preceding siblings ...)
2013-10-11 17:18 ` [PATCH 9/9] IB/core: extended command: move comp_mask to the extended header Yann Droneaud
@ 2013-10-14 15:36 ` Or Gerlitz
9 siblings, 0 replies; 31+ messages in thread
From: Or Gerlitz @ 2013-10-14 15:36 UTC (permalink / raw)
To: Yann Droneaud
Cc: Roland Dreier, Roland Dreier, linux-rdma-u79uwXL29TY76Z2rM5mHXA
On 11/10/2013 20:18, Yann Droneaud wrote:
> As requested by Or Gerlitz, I've rebased my previous fixes [1]
> against Matan Barak patch [2]. I've added two patches to fix issues
> left or introduced by this patch.
> Regarding the previous patchset [1], I've kept the public data
> structure improving and renaming patches, but, in order to minimize
> the overall changes, I've removed the changes which were modifying
> the functions and variables names in the code.
> I've removed "code beautifying" and left "hardening" / cleanup changes
> for a latter release.
>
> On top of this, I've added the two acknowledged and important patches
> from my extended command infrastructure [3]. I've made cleanups in the
> changelog regarding the 'email' citations.
Hi Yann, Matan provided some feedback, I think he has little more which
he will send tomorrow. Please make sure to use version-ing for the patch
series, e.g if I got it right, this post was V2 and the next is V3 -- so
you can just --subject-prefix="PATCH V3" for git format-patch, also
leave the listing of the change history on the cover letter e.g
V2 --> V3 changes:
xxx - what you will change in V3
V1 --> V2 changes:
yyy (what's written above)
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands
[not found] ` <0b84e20b204eb0fc67db9ca9106d6bf753a12ae3.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
@ 2013-10-14 15:38 ` Matan Barak
[not found] ` <525C0FF2.5040905-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2013-10-14 15:38 UTC (permalink / raw)
To: Yann Droneaud, Roland Dreier, Roland Dreier, Or Gerlitz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Igor Ivanov
On 11/10/2013 8:57 PM, Yann Droneaud wrote:
> Commit 400dbc9 added an infrastructure for extensible uverbs
> commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow()
> functions using this new infrastructure.
>
> According to the commit 400dbc9, the purpose of this infrastructure is
> to support passing around provider (eg. hardware) specific buffers
> when userspace issue commands to the kernel, so that it would be possible
> to extend uverbs (eg. core) buffers independently from the provider buffers.
>
> But the new kernel command function prototypes were not modified
> to take advantage of this extension. This issue was exposed by
> Roland Dreier in a previous review[1].
>
> So the following patch is an attempt to a revised extensible command
> infrastructure.
>
> This improved extensible command infrastructure distinguish between
> core (eg. legacy)'s command/response buffers from provider (eg. hardware)'s
> command/response buffers: each extended command implementing function
> is given a struct ib_udata to hold core (eg. uverbs) input and output buffers,
> and another struct ib_udata to hold the hw (eg. provider) input and output
> buffers.
> Having those buffers identified separately make it easier to increase one
> buffer to support extension without having to add some code to guess
> the exact size of each command/response parts: This should make the extended
> functions more reliable.
>
> Additionally, instead of relying on command identifier being greater
> than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure
> rely on unused bits in command field: on the 32 bits provided by
> command field, only 6 bits are really needed to encode the identifier
> of commands currently supported by the kernel. (Even using only 6 bits
> leaves room for about 23 new commands).
>
> So this patch makes use of some high order bits in command field
> to store flags, leaving enough room for more command identifiers
> than one will ever need (eg. 256).
>
> The new flags are used to specify if the command should be processed
> as an extended one or a legacy one. While designing the new command format,
> care was taken to make usage of flags itself extensible.
>
> Using high order bits of the commands field ensure
> that newer libibverbs on older kernel will properly fail
> when trying to call extended commands. On the other hand,
> older libibverbs on newer kernel will never be able to issue
> calls to extended commands.
>
> The extended command header includes the optional response
> pointer so that output buffer length and output buffer pointer
> are located together in the command, allowing proper parameters
> checking. This should make implementing functions easier and safer.
>
> Additionally the extended header ensure 64bits alignment,
> while making all sizes multiple of 8 bytes, extending
> the maximum buffer size:
>
> legacy extended
>
> Maximum command buffer: 256KBytes 1024KBytes (512KBytes + 512KBytes)
> Maximum response buffer: 256KBytes 1024KBytes (512KBytes + 512KBytes)
>
> For the purpose of doing proper buffer size accounting, the headers size
> are no more taken in account in "in_words".
>
> One of the odds of the current extensible infrastructure, reading twice
> the "legacy" command header, is fixed by removing the "legacy" command header
> from the extended command header: they are processed as two different parts
> of the command: memory is read once and information are not duplicated:
> it's making clear that's an extended command scheme and not a different
> command scheme.
>
> The proposed scheme will format input (command) and output (response)
> buffers this way:
>
> - command:
>
> legacy header +
> extended header +
> command data (core + hw):
>
> +----------------------------------------+
> | flags | 00 00 | command |
> | in_words | out_words |
> +----------------------------------------+
> | response |
> | response |
> | provider_in_words | provider_out_words |
> | padding |
> +----------------------------------------+
> | |
> . <uverbs input> .
> . (in_words * 8) .
> | |
> +----------------------------------------+
> | |
> . <provider input> .
> . (provider_in_words * 8) .
> | |
> +----------------------------------------+
>
> - response, if present:
>
> +----------------------------------------+
> | |
> . <uverbs output space> .
> . (out_words * 8) .
> | |
> +----------------------------------------+
> | |
> . <provider output space> .
> . (provider_out_words * 8) .
> | |
> +----------------------------------------+
>
> The overall design is to ensure that the extensible
> infrastructure is itself extensible while begin more
> reliable with more input and bound checking.
>
> [1]:
> http://marc.info/?iÊL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
>
> Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
> drivers/infiniband/core/uverbs.h | 18 ++++-
> drivers/infiniband/core/uverbs_cmd.c | 56 ++++++++--------
> drivers/infiniband/core/uverbs_main.c | 121 ++++++++++++++++++++++++++--------
> drivers/infiniband/hw/mlx4/main.c | 6 +-
> include/rdma/ib_verbs.h | 1 +
> include/uapi/rdma/ib_user_verbs.h | 21 +++---
> 6 files changed, 154 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index 4ae0307..bdc842e 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -47,6 +47,14 @@
> #include <rdma/ib_umem.h>
> #include <rdma/ib_user_verbs.h>
>
> +#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
> + do { \
> + (udata)->inbuf =void __user *) (ibuf); \
> + (udata)->outbuf =void __user *) (obuf); \
> + (udata)->inlen =ilen); \
> + (udata)->outlen =olen); \
> + } while (0)
> +
> /*
> * Our lifetime rules for these structs are the following:
> *
> @@ -233,7 +241,13 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
> IB_UVERBS_DECLARE_CMD(create_xsrq);
> IB_UVERBS_DECLARE_CMD(open_xrcd);
> IB_UVERBS_DECLARE_CMD(close_xrcd);
> -IB_UVERBS_DECLARE_CMD(create_flow);
> -IB_UVERBS_DECLARE_CMD(destroy_flow);
> +
> +#define IB_UVERBS_DECLARE_EX_CMD(name) \
> + int ib_uverbs_ex_##name(struct ib_uverbs_file *file, \
> + struct ib_udata *ucore, \
> + struct ib_udata *uhw)
> +
> +IB_UVERBS_DECLARE_EX_CMD(create_flow);
> +IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
>
> #endif /* UVERBS_H */
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 052e07c..e35877d 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -56,14 +56,6 @@ static struct uverbs_lock_class srq_lock_class = .name = "SRQ-uobj" };
> static struct uverbs_lock_class xrcd_lock_class = .name = "XRCD-uobj" };
> static struct uverbs_lock_class rule_lock_class = .name = "RULE-uobj" };
>
> -#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
> - do { \
> - (udata)->inbuf =void __user *) (ibuf); \
> - (udata)->outbuf =void __user *) (obuf); \
> - (udata)->inlen =ilen); \
> - (udata)->outlen =olen); \
> - } while (0)
> -
> /*
> * The ib_uobject locking scheme is as follows:
> *
> @@ -2639,9 +2631,9 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
> return 0;
> }
>
> -ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> - const char __user *buf, int in_len,
> - int out_len)
> +int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
> + struct ib_udata *ucore,
> + struct ib_udata *uhw)
> {
> struct ib_uverbs_create_flow cmd;
> struct ib_uverbs_create_flow_resp resp;
> @@ -2655,11 +2647,15 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> void *ib_spec;
> int i;
>
> - if (out_len < sizeof(resp))
> + if (ucore->outlen < sizeof(resp))
> return -ENOSPC;
>
> - if (copy_from_user(&cmd, buf, sizeof(cmd)))
> - return -EFAULT;
> + err =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
> + if (err)
> + return err;
> +
> + ucore->inbuf +=izeof(cmd);
> + ucore->inlen -=izeof(cmd);
>
> if (cmd.comp_mask)
> return -EINVAL;
> @@ -2671,7 +2667,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
> return -EINVAL;
>
> - if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
> + if (cmd.flow_attr.size > ucore->inlen ||
> cmd.flow_attr.size >
> (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
> return -EINVAL;
> @@ -2683,11 +2679,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> return -ENOMEM;
>
> memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
> - if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
> - cmd.flow_attr.size)) {
> - err =EFAULT;
> + err =b_copy_from_udata(kern_flow_attr + 1, ucore,
> + cmd.flow_attr.size);
> + if (err)
> goto err_free_attr;
> - }
> } else {
> kern_flow_attr =cmd.flow_attr;
> }
> @@ -2755,11 +2750,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> memset(&resp, 0, sizeof(resp));
> resp.flow_handle =obj->id;
>
> - if (copy_to_user((void __user *)(unsigned long) cmd.response,
> - &resp, sizeof(resp))) {
> - err =EFAULT;
> + err =b_copy_to_udata(ucore,
> + &resp, sizeof(resp));
> + if (err)
> goto err_copy;
> - }
>
> put_qp_read(qp);
> mutex_lock(&file->mutex);
> @@ -2772,7 +2766,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
> kfree(flow_attr);
> if (cmd.flow_attr.num_of_specs)
> kfree(kern_flow_attr);
> - return in_len;
> + return 0;
> err_copy:
> idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
> destroy_flow:
> @@ -2789,16 +2783,18 @@ err_free_attr:
> return err;
> }
>
> -ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
> - const char __user *buf, int in_len,
> - int out_len) {
> +int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
> + struct ib_udata *ucore,
> + struct ib_udata *uhw)
> +{
> struct ib_uverbs_destroy_flow cmd;
> struct ib_flow *flow_id;
> struct ib_uobject *uobj;
> int ret;
>
> - if (copy_from_user(&cmd, buf, sizeof(cmd)))
> - return -EFAULT;
> + ret =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
> + if (ret)
> + return ret;
>
> uobj =dr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
> file->ucontext);
> @@ -2820,7 +2816,7 @@ ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
>
> put_uobj(uobj);
>
> - return ret ? ret : in_len;
> + return ret ? ret : 0;
> }
>
> static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 75ad86c..4f00654 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -115,8 +115,13 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
> [IB_USER_VERBS_CMD_CLOSE_XRCD] =b_uverbs_close_xrcd,
> [IB_USER_VERBS_CMD_CREATE_XSRQ] =b_uverbs_create_xsrq,
> [IB_USER_VERBS_CMD_OPEN_QP] =b_uverbs_open_qp,
> - [IB_USER_VERBS_CMD_CREATE_FLOW] =b_uverbs_create_flow,
> - [IB_USER_VERBS_CMD_DESTROY_FLOW] =b_uverbs_destroy_flow
> +};
> +
> +static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
> + struct ib_udata *ucore,
> + struct ib_udata *uhw) =
> + [IB_USER_VERBS_EX_CMD_CREATE_FLOW] =b_uverbs_ex_create_flow,
> + [IB_USER_VERBS_EX_CMD_DESTROY_FLOW] =b_uverbs_ex_destroy_flow
> };
>
> static void ib_uverbs_add_one(struct ib_device *device);
> @@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> {
> struct ib_uverbs_file *file =ilp->private_data;
> struct ib_uverbs_cmd_hdr hdr;
> + __u32 flags;
>
> if (count < sizeof hdr)
> return -EINVAL;
> @@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> if (copy_from_user(&hdr, buf, sizeof hdr))
> return -EFAULT;
>
> - if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
> - !uverbs_cmd_table[hdr.command])
> - return -EINVAL;
> + flags =hdr.command &
> + IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>
> - if (!file->ucontext &&
> - hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
> - return -EINVAL;
> + if (!flags) {
> + __u32 command;
>
> - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
> - return -ENOSYS;
> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> + IB_USER_VERBS_CMD_COMMAND_MASK))
> + return -EINVAL;
>
> - if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
> - struct ib_uverbs_cmd_hdr_ex hdr_ex;
> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>
> - if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
> - return -EFAULT;
> + if (command >=RRAY_SIZE(uverbs_cmd_table) ||
> + !uverbs_cmd_table[command])
> + return -EINVAL;
>
> - if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4) !=ount)
> + if (!file->ucontext &&
> + command !=B_USER_VERBS_CMD_GET_CONTEXT)
> return -EINVAL;
>
> - return uverbs_cmd_table[hdr.command](file,
> - buf + sizeof(hdr_ex),
> - (hdr_ex.in_words +
> - hdr_ex.provider_in_words) * 4,
> - (hdr_ex.out_words +
> - hdr_ex.provider_out_words) * 4);
> - } else {
> + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command)))
> + return -ENOSYS;
> +
> if (hdr.in_words * 4 !=ount)
> return -EINVAL;
>
> - return uverbs_cmd_table[hdr.command](file,
> - buf + sizeof(hdr),
> - hdr.in_words * 4,
> - hdr.out_words * 4);
> + return uverbs_cmd_table[command](file,
> + buf + sizeof(hdr),
> + hdr.in_words * 4,
> + hdr.out_words * 4);
> +
> + } else if (flags =IB_USER_VERBS_CMD_FLAG_EXTENDED) {
> + __u32 command;
> +
> + struct ib_uverbs_ex_cmd_hdr ex_hdr;
> + struct ib_udata ucore;
> + struct ib_udata uhw;
> + int err;
> +
> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> + IB_USER_VERBS_CMD_COMMAND_MASK))
> + return -EINVAL;
> +
> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> +
> + if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
> + !uverbs_ex_cmd_table[command])
> + return -ENOSYS;
> +
> + if (!file->ucontext)
> + return -EINVAL;
> +
> + if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command)))
> + return -ENOSYS;
> +
> + if (count < (sizeof(hdr) + sizeof(ex_hdr)))
> + return -EINVAL;
> +
> + if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr)))
> + return -EFAULT;
> +
> + count -=izeof(hdr) + sizeof(ex_hdr);
> + buf +=izeof(hdr) + sizeof(ex_hdr);
> +
> + if ((hdr.in_words + ex_hdr.provider_in_words) * 8 !=ount)
> + return -EINVAL;
> +
> + if (ex_hdr.response) {
> + if (!hdr.out_words && !ex_hdr.provider_out_words)
> + return -EINVAL;
> + } else {
> + if (hdr.out_words || ex_hdr.provider_out_words)
> + return -EINVAL;
> + }
> +
> + INIT_UDATA(&ucore,
> + (hdr.in_words) ? buf : 0,
> + (unsigned long)ex_hdr.response,
> + hdr.in_words * 8,
> + hdr.out_words * 8);
> +
> + INIT_UDATA(&uhw,
> + (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
> + (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0,
> + ex_hdr.provider_in_words * 8,
> + ex_hdr.provider_out_words * 8);
> +
> + err =verbs_ex_cmd_table[command](file,
> + &ucore,
> + &uhw);
> +
> + if (err)
> + return err;
> +
> + return count;
> }
> +
> + return -ENOSYS;
> }
>
> static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index d6c5a73..1aad9b3 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -1691,9 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
> ibdev->ib_dev.create_flow =lx4_ib_create_flow;
> ibdev->ib_dev.destroy_flow =lx4_ib_destroy_flow;
>
> - ibdev->ib_dev.uverbs_cmd_mask |- (1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |
> - (1ull << IB_USER_VERBS_CMD_DESTROY_FLOW);
> + ibdev->ib_dev.uverbs_ex_cmd_mask |+ (1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |
> + (1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
> }
>
> mlx4_ib_alloc_eqs(dev, ibdev);
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index e393171..a06fc12 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1436,6 +1436,7 @@ struct ib_device {
>
> int uverbs_abi_ver;
> u64 uverbs_cmd_mask;
> + u64 uverbs_ex_cmd_mask;
>
> char node_desc[64];
> __be64 node_guid;
> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
> index 93577fc..cbfdd4c 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -87,8 +87,11 @@ enum {
> IB_USER_VERBS_CMD_CLOSE_XRCD,
> IB_USER_VERBS_CMD_CREATE_XSRQ,
> IB_USER_VERBS_CMD_OPEN_QP,
> - IB_USER_VERBS_CMD_CREATE_FLOW =B_USER_VERBS_CMD_THRESHOLD,
> - IB_USER_VERBS_CMD_DESTROY_FLOW
> +};
> +
> +enum {
> + IB_USER_VERBS_EX_CMD_CREATE_FLOW =B_USER_VERBS_CMD_THRESHOLD,
> + IB_USER_VERBS_EX_CMD_DESTROY_FLOW
> };
I think B_USER_VERBS_CMD_THRESHOLD could be removed.
>
> /*
> @@ -120,16 +123,20 @@ struct ib_uverbs_comp_event_desc {
> * the rest of the command struct based on these value.
> */
>
> +#define IB_USER_VERBS_CMD_COMMAND_MASK 0xff
> +#define IB_USER_VERBS_CMD_FLAGS_MASK 0xff000000u
> +#define IB_USER_VERBS_CMD_FLAGS_SHIFT 24
> +
> +#define IB_USER_VERBS_CMD_FLAG_EXTENDED 0x80
> +
> struct ib_uverbs_cmd_hdr {
> __u32 command;
> __u16 in_words;
> __u16 out_words;
> };
>
> -struct ib_uverbs_cmd_hdr_ex {
> - __u32 command;
> - __u16 in_words;
> - __u16 out_words;
> +struct ib_uverbs_ex_cmd_hdr {
> + __u64 response;
> __u16 provider_in_words;
> __u16 provider_out_words;
> __u32 cmd_hdr_reserved;
> @@ -777,8 +784,6 @@ struct ib_uverbs_flow_attr {
>
> struct ib_uverbs_create_flow {
> __u32 comp_mask;
> - __u32 reserved;
> - __u64 response;
> __u32 qp_handle;
> struct ib_uverbs_flow_attr flow_attr;
> };
> --
> 1.8.3.1
>
This infrastructure (as well as the original one) doesn't deal with the
case we discussed about in the previous patches. When a command contains
a structure that can be extended, we don't have a clean nice way of
doing that. Though, because it's a limitation of the original design and
this patch cleans up the functionality of the original infrastructure,
I'm in favor of merging it.
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 9/9] IB/core: extended command: move comp_mask to the extended header
[not found] ` <40175eda10d670d098204da6aa4c327a0171ae5f.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
@ 2013-10-14 15:44 ` Matan Barak
0 siblings, 0 replies; 31+ messages in thread
From: Matan Barak @ 2013-10-14 15:44 UTC (permalink / raw)
To: Yann Droneaud, Roland Dreier, Roland Dreier, Or Gerlitz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Igor Ivanov
On 11/10/2013 8:57 PM, Yann Droneaud wrote:
> The unused field in the extended header is a perfect candidate
> to hold the command "comp_mask" (eg. bit field used to handle
> compatibility). This was suggested by Roland Dreier in a previous
> review[1].
>
> So this patch move comp_mask from create_flow/destroy_flow commands
> to the extended command header. Then comp_mask is passed as part
> of function parameters.
>
> [1]:
> http://marc.info/?iÊL1RGDXJtrc849M6_XNZT5xO1+ybKtLWGq6yg6LhoSsKpsmkYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
>
> Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
> drivers/infiniband/core/uverbs.h | 3 ++-
> drivers/infiniband/core/uverbs_cmd.c | 15 ++++++++++-----
> drivers/infiniband/core/uverbs_main.c | 6 ++++--
> include/uapi/rdma/ib_user_verbs.h | 6 +++---
> 4 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index bdc842e..a12b516 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -245,7 +245,8 @@ IB_UVERBS_DECLARE_CMD(close_xrcd);
> #define IB_UVERBS_DECLARE_EX_CMD(name) \
> int ib_uverbs_ex_##name(struct ib_uverbs_file *file, \
> struct ib_udata *ucore, \
> - struct ib_udata *uhw)
> + struct ib_udata *uhw, \
> + __u32 comp_mask)
>
> IB_UVERBS_DECLARE_EX_CMD(create_flow);
> IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index e35877d..a8ecb3a 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2633,7 +2633,8 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
>
> int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
> struct ib_udata *ucore,
> - struct ib_udata *uhw)
> + struct ib_udata *uhw,
> + __u32 comp_mask)
> {
> struct ib_uverbs_create_flow cmd;
> struct ib_uverbs_create_flow_resp resp;
> @@ -2647,6 +2648,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
> void *ib_spec;
> int i;
>
> + if (comp_mask)
> + return -EINVAL;
> +
> if (ucore->outlen < sizeof(resp))
> return -ENOSPC;
>
> @@ -2657,9 +2661,6 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
> ucore->inbuf +=izeof(cmd);
> ucore->inlen -=izeof(cmd);
>
> - if (cmd.comp_mask)
> - return -EINVAL;
> -
> if ((cmd.flow_attr.type =IB_FLOW_ATTR_SNIFFER &&
> !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
> return -EPERM;
> @@ -2785,13 +2786,17 @@ err_free_attr:
>
> int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
> struct ib_udata *ucore,
> - struct ib_udata *uhw)
> + struct ib_udata *uhw,
> + __u32 comp_mask)
> {
> struct ib_uverbs_destroy_flow cmd;
> struct ib_flow *flow_id;
> struct ib_uobject *uobj;
> int ret;
>
> + if (comp_mask)
> + return -EINVAL;
> +
> ret =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
> if (ret)
> return ret;
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 4f00654..a9687dd 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -119,7 +119,8 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
>
> static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
> struct ib_udata *ucore,
> - struct ib_udata *uhw) =
> + struct ib_udata *uhw,
> + __u32 comp_mask) =
> [IB_USER_VERBS_EX_CMD_CREATE_FLOW] =b_uverbs_ex_create_flow,
> [IB_USER_VERBS_EX_CMD_DESTROY_FLOW] =b_uverbs_ex_destroy_flow
> };
> @@ -689,7 +690,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>
> err =verbs_ex_cmd_table[command](file,
> &ucore,
> - &uhw);
> + &uhw,
> + ex_hdr.comp_mask);
>
> if (err)
> return err;
> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
> index cbfdd4c..095a2bd 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -139,7 +139,7 @@ struct ib_uverbs_ex_cmd_hdr {
> __u64 response;
> __u16 provider_in_words;
> __u16 provider_out_words;
> - __u32 cmd_hdr_reserved;
> + __u32 comp_mask;
> };
>
> struct ib_uverbs_get_context {
> @@ -783,8 +783,8 @@ struct ib_uverbs_flow_attr {
> };
>
> struct ib_uverbs_create_flow {
> - __u32 comp_mask;
> __u32 qp_handle;
> + __u32 reserved;
> struct ib_uverbs_flow_attr flow_attr;
> };
>
> @@ -794,8 +794,8 @@ struct ib_uverbs_create_flow_resp {
> };
>
> struct ib_uverbs_destroy_flow {
> - __u32 comp_mask;
> __u32 flow_handle;
> + __u32 reserved;
> };
>
> struct ib_uverbs_create_srq {
> --
> 1.8.3.1
>
I think that's a good idea to make the comp_mask field a part of our
infrastructure. But, I think we should consider making this symmetrical.
If we use the command's comp_mask as an input parameter, shouldn't we
use the response's comp_mask as an output parameter ?
What's about the provider's comp_mask ?
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <525C0BF2.2060201-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-10-14 15:46 ` Yann Droneaud
[not found] ` <72b95284386dc57dfa1c7c883b4f45af-zgzEX58YAwA@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-14 15:46 UTC (permalink / raw)
To: Matan Barak
Cc: Roland Dreier, Roland Dreier, Or Gerlitz,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hi,
Le 14.10.2013 17:21, Matan Barak a écrit :
> On 11/10/2013 8:19 PM, Yann Droneaud wrote:
>> Flow spec length don't depend on the size of the command
>> header: they are part of different layers.
>>
>> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
>> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>> ---
>> drivers/infiniband/core/uverbs_cmd.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c
>> b/drivers/infiniband/core/uverbs_cmd.c
>> index 3b732f6..1e5f0dd 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>> @@ -2671,8 +2671,8 @@ ssize_t ib_uverbs_create_flow(struct
>> ib_uverbs_file *file,
>> if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
>> return -EINVAL;
>>
>> - if (cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
>> - ib_uverbs_cmd_hdr_ex)) || cmd.flow_attr.size >
>> + if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
>> + cmd.flow_attr.size >
>> (cmd.flow_attr.num_of_specs * sizeof(struct
>> ib_kern_spec)))
>> return -EINVAL;
>>
>> --
>> 1.8.3.1
>>
>
> Hi,
>
> I agree that it was better if the command size we pass to the uverb
> didn't include the header size, but since the current implementation
> of ib_uverbs_write and its user-space counterpart passes the command
> size including the header - I think this fix is incorrect. I think
> it'll actually break the current implementation.
You're correct. I'm wrong. I broke this.
I'm so wrong, I've sent a whole patchset that *didn't* take this into
account.
See http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
I had to re-read it again:
return uverbs_cmd_table[hdr.command](file, buf + sizeof hdr,
hdr.in_words * 4,
hdr.out_words * 4);
Seems I'm not mentally trained to accept that the input buffer size
is larger than buffer it's related to. I keep thinking it's written
return uverbs_cmd_table[hdr.command](file, buf + sizeof hdr,
hdr.in_words * 4 - sizeof
hdr, hdr.out_words * 4);
And that's what I've proposed in the extensible infrastructure.
> Furthermore, patch 8 overwrites this anyway, so I guess we could
> reduce this patchset size by removing it.
>
I will follow your advice.
Let's try for inclusion in v3.12-rc6
Regards.
--
Yann Droneaud
OPTEYA
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <72b95284386dc57dfa1c7c883b4f45af-zgzEX58YAwA@public.gmane.org>
@ 2013-10-14 18:19 ` Roland Dreier
[not found] ` <CAL1RGDWarJPu_6u5oaL6NgZZJXBuB09vTi9xhxHALfiY_W9Sgg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Roland Dreier @ 2013-10-14 18:19 UTC (permalink / raw)
To: Yann Droneaud
Cc: Matan Barak, Or Gerlitz,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Mon, Oct 14, 2013 at 8:46 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> Let's try for inclusion in v3.12-rc6
Hi everyone. First of sorry for being incommunicado for so long, been
swamped with a lot of stuff.
Anyway, given that everyone seems to agree we want something along the
lines of this series, but that the series is not ready yet, it really
seems to be that at this point the best option is Yann's simple patch
to temporarily disable these commands.
Can anyone talk me out of that?
- R.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <CAL1RGDWarJPu_6u5oaL6NgZZJXBuB09vTi9xhxHALfiY_W9Sgg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-10-15 14:08 ` Yann Droneaud
[not found] ` <122f1a29d6fe45449a6a153a3a9ba8b1-zgzEX58YAwA@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-15 14:08 UTC (permalink / raw)
To: Roland Dreier; +Cc: Matan Barak, Or Gerlitz, linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hi Roland,
Le 14.10.2013 20:19, Roland Dreier a écrit :
> On Mon, Oct 14, 2013 at 8:46 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> wrote:
>> Let's try for inclusion in v3.12-rc6
>
> Hi everyone. First of sorry for being incommunicado for so long, been
> swamped with a lot of stuff.
>
> Anyway, given that everyone seems to agree we want something along the
> lines of this series, but that the series is not ready yet, it really
> seems to be that at this point the best option is Yann's simple patch
> to temporarily disable these commands.
>
> Can anyone talk me out of that?
>
From my point of view:
- commit 400dbc9 "IB/core: Infrastructure for extensible uverbs
commands"
was introduced with the purpose of providing ground for new commands,
but fail to actually provide real benefit, it just seems complicated
for nothing.
In fact, when reading the commit, I think the code is not doing
what's advertised in the commit message and in the extended command
header:
What's the point of splitting "core" from "provider" buffer ?
- commit 436f2ad "IB/core: Export ib_create/destroy_flow through uverbs"
rely on the extensible uverbs infrastructure introduced in commit
400dbc9, but it's not clear why it needs the infrastructure,
perhaps bigger command buffer is needed for large set of flow_spec ?
More important, ABI/API exposed would welcome some cleanups:
- The public data structures need a bit of rework: renaming, removing
struct ib_kern_spec
- The size of the flow_spec list is computed by subtracting the
command
header length and command to the total length.
kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
sizeof(struct ib_uverbs_cmd_hdr_ex);
The size must not depend on the command header, I believe they are
part
of different layer. cmd.flow_attr.size should be the size of the
flow_spec list.
- The parsing flow_spec is a bit incorrect, it goes wrong at
manipulating the size
of flow_spec list. Additionally it should not rely on underflow
kern_attr_size to detect error.
- commit 22878db "IB/core: Better checking of userspace values for
receive flow steering"
add checks for negative value on unsigned variables
- patch https://patchwork.kernel.org/patch/2924341/
"IB/core: clarify overflow/underflow checks on ib_create/destroy_flow"
is improving ib_uverbs_create_flow() and ib_uverbs_destroy_flow()
to fix the remarks I've made against commit 436f2ad and 22878db
but introduce a bug, not adding the size of flow_attr, when allocating
memory
for the flow_attr + flow_specs.
- Additionally, the improved extensible command scheme require changes
in ib_uverbs_create_flow() and ib_uverbs_destroy_flow() function
to use a new prototypes.
I've proposed fixe for ib_uverbs_create_flow(), ib_uverbs_destroy_flow()
and public API.
I've proposed an improvement of the extensible scheme based on my
understanding
of the intent described in the commit 400dbc9 message. This scheme seems
better,
but it's not the silver bullet: it doesn't address new requirements
provided by Matan Barak:
http://marc.info/?i=524AB54F.9050205-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> In addition, commands that have extensible sub-structures (for
example,
> extended address handle in QP attributes in IP based addressing
patch)
> should be given as a different UDATA in the cmd structure.
Therefore, we
> need a comp_mask in the cmd structure header to describe which UDATA
> structure are included.
http://marc.info/?i=5253F37C.90508-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> Regarding the last patch, you are right that it simplifies things
for
> creating new uverbs where command parts are in-lined one after
another,
> but the infrastructure got a bit more complex.
> If we're going to this direction, I think the we should also deal
with
> the problem of extending one of the command parts. Currently, we'll
have
> to put a comp_mask in the in-lined command part, consume this
command
> part and then continue with the other parts. It might be better than
> using a pointer, but this put the burden of serializing the command
> buffer into the kernel structures onto the uverb command writer.
> We might want to avoid this.
> Furthermore, the comp_mask of the command is different than the
> comp_mask of the response. Therefore, I don't think we should pass
the
> command's comp_mask to the uverb as a pointer, but just pass a
pointer
> to value 0 that the uverb will set.
http://marc.info/?i=525C1149.6000701-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> I think that's a good idea to make the comp_mask field a part of our
> infrastructure. But, I think we should consider making this
symmetrical.
> If we use the command's comp_mask as an input parameter, shouldn't
we
> use the response's comp_mask as an output parameter ?
> What's about the provider's comp_mask ?
http://marc.info/?i=525C0FF2.5040905-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org
> This infrastructure (as well as the original one) doesn't deal with
the
> case we discussed about in the previous patches. When a command
contains
> a structure that can be extended, we don't have a clean nice way of
> doing that. Though, because it's a limitation of the original design
and
> this patch cleans up the functionality of the original
infrastructure,
> I'm in favor of merging it.
I would summarize:
- having the ability to extend multiple part of the command,
independently of the other;
- same for the response;
- having comp_mask in command for uverbs (eg. core/) and for provider
(eg. hw/);
- same for the response.
I've provided a patch to handle "comp_mask" in response,
but I don't feel confident in it: design doesn't sound clean.
While I would like to address the unlimited command expansion problem,
I think the scheme I've proposed is the wrong way. But in the same time
I don't believe I have a good way to do it.
The proposed new scheme is only good for core / hw split.
But, in the mean time, we failed at providing the required
fixes/cleanups before v3.12-rc5.
We have not agreed on a definitive, small, urgent patchset for
v3.12-rc5.
And I think, and it's easy to think so, that Linus will be very upset to
see more
than a couple of patches for the Infiniband subsystem for v3.12-rc6. If
there's a v3.12-rc6.
So instead of fighting to squeeze the last good patch hunk for v3.12, we
should think
on what command/response scheme we need for v3.13.
Regards.
--
Yann Droneaud
OPTEYA
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <122f1a29d6fe45449a6a153a3a9ba8b1-zgzEX58YAwA@public.gmane.org>
@ 2013-10-15 16:44 ` Roland Dreier
[not found] ` <CAL1RGDXP3+X=go4GiW_SsvSvC_VS-B7uacJN0u5wVtU3NWfvQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Roland Dreier @ 2013-10-15 16:44 UTC (permalink / raw)
To: Yann Droneaud
Cc: Matan Barak, Or Gerlitz,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Tue, Oct 15, 2013 at 7:08 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> But, in the mean time, we failed at providing the required fixes/cleanups
> before v3.12-rc5.
> We have not agreed on a definitive, small, urgent patchset for v3.12-rc5.
> And I think, and it's easy to think so, that Linus will be very upset to see
> more
> than a couple of patches for the Infiniband subsystem for v3.12-rc6. If
> there's a v3.12-rc6.
>
> So instead of fighting to squeeze the last good patch hunk for v3.12, we
> should think
> on what command/response scheme we need for v3.13.
I agree. So as I said before, I think the best idea is just to
disable the half-baked command extension stuff before 3.12, and get
things right for 3.13.
Do you still think the patch you provided to disable the new stuff is good?
- R.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <CAL1RGDXP3+X=go4GiW_SsvSvC_VS-B7uacJN0u5wVtU3NWfvQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-10-15 17:07 ` Yann Droneaud
2013-10-15 21:34 ` Or Gerlitz
1 sibling, 0 replies; 31+ messages in thread
From: Yann Droneaud @ 2013-10-15 17:07 UTC (permalink / raw)
To: Roland Dreier; +Cc: Matan Barak, Or Gerlitz, linux-rdma-u79uwXL29TY76Z2rM5mHXA
Hi Roland,
Le 15.10.2013 18:44, Roland Dreier a écrit :
> On Tue, Oct 15, 2013 at 7:08 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> wrote:
>> But, in the mean time, we failed at providing the required
>> fixes/cleanups
>> before v3.12-rc5.
>> We have not agreed on a definitive, small, urgent patchset for
>> v3.12-rc5.
>> And I think, and it's easy to think so, that Linus will be very upset
>> to see
>> more
>> than a couple of patches for the Infiniband subsystem for v3.12-rc6.
>> If
>> there's a v3.12-rc6.
>>
>> So instead of fighting to squeeze the last good patch hunk for v3.12,
>> we
>> should think
>> on what command/response scheme we need for v3.13.
>
> I agree. So as I said before, I think the best idea is just to
> disable the half-baked command extension stuff before 3.12, and get
> things right for 3.13.
>
At least, every voices seems to acknowledge that the current command
extension scheme need some work. But I have the feeling the amount
of work is not known.
> Do you still think the patch you provided to disable the new stuff is
> good?
>
This one : https://patchwork.kernel.org/patch/3015021/
I don't think is good ... adding a bunch of #if 0 / #endif is never
good.
But it's smaller than a revert and, perhaps, politically more appealing.
Regarding the technical details, the kernel compiles without warning.
It was "NACK"'ed by Or Gerlitz who would like to see my improvements
applied
ASAP. But in the mean time Matan Barack expressed concerns over the
scheme as
it is currently, even with my improvements.
YMMV.
Regards.
--
Yann Droneaud
OPTEYA
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <CAL1RGDXP3+X=go4GiW_SsvSvC_VS-B7uacJN0u5wVtU3NWfvQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-10-15 17:07 ` Yann Droneaud
@ 2013-10-15 21:34 ` Or Gerlitz
[not found] ` <CAJZOPZLr41MGb7qPHcFVQxNcwwAB8i4vnGQhEcu0oZ-BOiMBcg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
1 sibling, 1 reply; 31+ messages in thread
From: Or Gerlitz @ 2013-10-15 21:34 UTC (permalink / raw)
To: Roland Dreier
Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Tue, Oct 15, 2013 at 7:44 PM, Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org> wrote:
> I agree. So as I said before, I think the best idea is just to
> disable the half-baked command extension stuff before 3.12, and get
> things right for 3.13.
Roland, after applying a small fix which was sent to you by Matan on
Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be
surely squashed into one small patch), things are working OK. Yann is
suggesting to enhance them to work and look even better and for some
reasons (life) he didn't get to do that on the 3-4 months the patches
spent on the list and went through review of Sean/Jason/Shawn and you
(RD). I don't see how/why you use the term half-baked here.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <CAJZOPZLr41MGb7qPHcFVQxNcwwAB8i4vnGQhEcu0oZ-BOiMBcg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-10-16 0:04 ` Roland Dreier
[not found] ` <CAL1RGDW7KwSOmmsRxDokUHR-Mh1OzaQvDege5Rq7JsSU8AgEHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Roland Dreier @ 2013-10-16 0:04 UTC (permalink / raw)
To: Or Gerlitz
Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On Tue, Oct 15, 2013 at 2:34 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Roland, after applying a small fix which was sent to you by Matan on
> Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be
> surely squashed into one small patch), things are working OK. Yann is
> suggesting to enhance them to work and look even better and for some
> reasons (life) he didn't get to do that on the 3-4 months the patches
> spent on the list and went through review of Sean/Jason/Shawn and you
> (RD). I don't see how/why you use the term half-baked here.
Unless I'm misunderstanding, Yann's suggestion for enhancement is not
user/kernel ABI compatible with what is in the tree now. Given that
we should really try to find an ABI that lasts for a long time, and
that we already have a proposal to improve things, shouldn't we hold
off on freezing the new ABI for one more kernel cycle?
- R.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <CAL1RGDW7KwSOmmsRxDokUHR-Mh1OzaQvDege5Rq7JsSU8AgEHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2013-10-17 9:19 ` Or Gerlitz
[not found] ` <525FABA2.6000507-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Or Gerlitz @ 2013-10-17 9:19 UTC (permalink / raw)
To: Roland Dreier, Yann Droneaud
Cc: Tzahi Oved, Matan Barak,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
On 16/10/2013 03:04, Roland Dreier wrote:
> On Tue, Oct 15, 2013 at 2:34 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Roland, after applying a small fix which was sent to you by Matan on
>> Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be
>> surely squashed into one small patch), things are working OK. Yann is
>> suggesting to enhance them to work and look even better and for some
>> reasons (life) he didn't get to do that on the 3-4 months the patches
>> spent on the list and went through review of Sean/Jason/Shawn and you
>> (RD). I don't see how/why you use the term half-baked here.
> Unless I'm misunderstanding, Yann's suggestion for enhancement is not
> user/kernel ABI compatible with what is in the tree now. Given that
> we should really try to find an ABI that lasts for a long time, and
> that we already have a proposal to improve things, shouldn't we hold
> off on freezing the new ABI for one more kernel cycle?
>
>
Roland, Yann, the uverbs extensions + flow-steering patches spent many
months on the list, once the review started
we did quick fixes for all the comments and made it for 3.12-rc1 which
was delay or two kernel cycles vs. what
could happen if the feedback was on time (*).
Indeed since we do want to have ABI that lasts for long, deferring the
uverbs support for extensions to 3.13 along with
flow-steering support to user space is something we can live with,
conditioned on commitment of Yann to quickly submit
his fixed patches that addresses the issues Matan raised and of you (RD)
to pick them once Yann and Matan agree
its done into for-next so they are safe for 3.13
Just to double check, by no means we are talking on reverting the whole
series, flow-steering will be in
3.12 in the IB core and mlx4_ib
OK?!
Or.
(*) ~Ditto for the IP based addressing series, I sent you detailed reply
to your comments > month ago and not
a word from you, so another kernel cycles is going to be wasted there too.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands
[not found] ` <525C0FF2.5040905-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-10-17 14:41 ` Matan Barak
[not found] ` <525FF724.5050601-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Matan Barak @ 2013-10-17 14:41 UTC (permalink / raw)
To: Yann Droneaud, Roland Dreier, Roland Dreier, Or Gerlitz
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Igor Ivanov
On 14/10/2013 6:38 PM, Matan Barak wrote:
> On 11/10/2013 8:57 PM, Yann Droneaud wrote:
>> Commit 400dbc9 added an infrastructure for extensible uverbs
>> commands while later commit 436f2ad exported
>> ib_create_flow()/ib_destroy_flow()
>> functions using this new infrastructure.
>>
>> According to the commit 400dbc9, the purpose of this infrastructure is
>> to support passing around provider (eg. hardware) specific buffers
>> when userspace issue commands to the kernel, so that it would be possible
>> to extend uverbs (eg. core) buffers independently from the provider
>> buffers.
>>
>> But the new kernel command function prototypes were not modified
>> to take advantage of this extension. This issue was exposed by
>> Roland Dreier in a previous review[1].
>>
>> So the following patch is an attempt to a revised extensible command
>> infrastructure.
>>
>> This improved extensible command infrastructure distinguish between
>> core (eg. legacy)'s command/response buffers from provider (eg.
>> hardware)'s
>> command/response buffers: each extended command implementing function
>> is given a struct ib_udata to hold core (eg. uverbs) input and output
>> buffers,
>> and another struct ib_udata to hold the hw (eg. provider) input and
>> output
>> buffers.
>> Having those buffers identified separately make it easier to increase one
>> buffer to support extension without having to add some code to guess
>> the exact size of each command/response parts: This should make the
>> extended
>> functions more reliable.
>>
>> Additionally, instead of relying on command identifier being greater
>> than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure
>> rely on unused bits in command field: on the 32 bits provided by
>> command field, only 6 bits are really needed to encode the identifier
>> of commands currently supported by the kernel. (Even using only 6 bits
>> leaves room for about 23 new commands).
>>
>> So this patch makes use of some high order bits in command field
>> to store flags, leaving enough room for more command identifiers
>> than one will ever need (eg. 256).
>>
>> The new flags are used to specify if the command should be processed
>> as an extended one or a legacy one. While designing the new command
>> format,
>> care was taken to make usage of flags itself extensible.
>>
>> Using high order bits of the commands field ensure
>> that newer libibverbs on older kernel will properly fail
>> when trying to call extended commands. On the other hand,
>> older libibverbs on newer kernel will never be able to issue
>> calls to extended commands.
>>
>> The extended command header includes the optional response
>> pointer so that output buffer length and output buffer pointer
>> are located together in the command, allowing proper parameters
>> checking. This should make implementing functions easier and safer.
>>
>> Additionally the extended header ensure 64bits alignment,
>> while making all sizes multiple of 8 bytes, extending
>> the maximum buffer size:
>>
>> legacy extended
>>
>> Maximum command buffer: 256KBytes 1024KBytes (512KBytes +
>> 512KBytes)
>> Maximum response buffer: 256KBytes 1024KBytes (512KBytes +
>> 512KBytes)
>>
>> For the purpose of doing proper buffer size accounting, the headers size
>> are no more taken in account in "in_words".
>>
>> One of the odds of the current extensible infrastructure, reading twice
>> the "legacy" command header, is fixed by removing the "legacy" command
>> header
>> from the extended command header: they are processed as two different
>> parts
>> of the command: memory is read once and information are not duplicated:
>> it's making clear that's an extended command scheme and not a different
>> command scheme.
>>
>> The proposed scheme will format input (command) and output (response)
>> buffers this way:
>>
>> - command:
>>
>> legacy header +
>> extended header +
>> command data (core + hw):
>>
>> +----------------------------------------+
>> | flags | 00 00 | command |
>> | in_words | out_words |
>> +----------------------------------------+
>> | response |
>> | response |
>> | provider_in_words | provider_out_words |
>> | padding |
>> +----------------------------------------+
>> | |
>> . <uverbs input> .
>> . (in_words * 8) .
>> | |
>> +----------------------------------------+
>> | |
>> . <provider input> .
>> . (provider_in_words * 8) .
>> | |
>> +----------------------------------------+
>>
>> - response, if present:
>>
>> +----------------------------------------+
>> | |
>> . <uverbs output space> .
>> . (out_words * 8) .
>> | |
>> +----------------------------------------+
>> | |
>> . <provider output space> .
>> . (provider_out_words * 8) .
>> | |
>> +----------------------------------------+
>>
>> The overall design is to ensure that the extensible
>> infrastructure is itself extensible while begin more
>> reliable with more input and bound checking.
>>
>> [1]:
>> http://marc.info/?iÊL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
>>
>>
>> Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
>> Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
>> Link: http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>> Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>> ---
>> drivers/infiniband/core/uverbs.h | 18 ++++-
>> drivers/infiniband/core/uverbs_cmd.c | 56 ++++++++--------
>> drivers/infiniband/core/uverbs_main.c | 121
>> ++++++++++++++++++++++++++--------
>> drivers/infiniband/hw/mlx4/main.c | 6 +-
>> include/rdma/ib_verbs.h | 1 +
>> include/uapi/rdma/ib_user_verbs.h | 21 +++---
>> 6 files changed, 154 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/uverbs.h
>> b/drivers/infiniband/core/uverbs.h
>> index 4ae0307..bdc842e 100644
>> --- a/drivers/infiniband/core/uverbs.h
>> +++ b/drivers/infiniband/core/uverbs.h
>> @@ -47,6 +47,14 @@
>> #include <rdma/ib_umem.h>
>> #include <rdma/ib_user_verbs.h>
>>
>> +#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
>> + do { \
>> + (udata)->inbuf =void __user *) (ibuf); \
>> + (udata)->outbuf =void __user *) (obuf); \
>> + (udata)->inlen =ilen); \
>> + (udata)->outlen =olen); \
>> + } while (0)
>> +
>> /*
>> * Our lifetime rules for these structs are the following:
>> *
>> @@ -233,7 +241,13 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
>> IB_UVERBS_DECLARE_CMD(create_xsrq);
>> IB_UVERBS_DECLARE_CMD(open_xrcd);
>> IB_UVERBS_DECLARE_CMD(close_xrcd);
>> -IB_UVERBS_DECLARE_CMD(create_flow);
>> -IB_UVERBS_DECLARE_CMD(destroy_flow);
>> +
>> +#define IB_UVERBS_DECLARE_EX_CMD(name) \
>> + int ib_uverbs_ex_##name(struct ib_uverbs_file *file, \
>> + struct ib_udata *ucore, \
>> + struct ib_udata *uhw)
>> +
>> +IB_UVERBS_DECLARE_EX_CMD(create_flow);
>> +IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
>>
>> #endif /* UVERBS_H */
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c
>> b/drivers/infiniband/core/uverbs_cmd.c
>> index 052e07c..e35877d 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
>> @@ -56,14 +56,6 @@ static struct uverbs_lock_class
>> srq_lock_class = .name = "SRQ-uobj" };
>> static struct uverbs_lock_class xrcd_lock_class = .name =
>> "XRCD-uobj" };
>> static struct uverbs_lock_class rule_lock_class = .name =
>> "RULE-uobj" };
>>
>> -#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \
>> - do { \
>> - (udata)->inbuf =void __user *) (ibuf); \
>> - (udata)->outbuf =void __user *) (obuf); \
>> - (udata)->inlen =ilen); \
>> - (udata)->outlen =olen); \
>> - } while (0)
>> -
>> /*
>> * The ib_uobject locking scheme is as follows:
>> *
>> @@ -2639,9 +2631,9 @@ static int kern_spec_to_ib_spec(struct
>> ib_uverbs_flow_spec *kern_spec,
>> return 0;
>> }
>>
>> -ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
>> - const char __user *buf, int in_len,
>> - int out_len)
>> +int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>> + struct ib_udata *ucore,
>> + struct ib_udata *uhw)
>> {
>> struct ib_uverbs_create_flow cmd;
>> struct ib_uverbs_create_flow_resp resp;
>> @@ -2655,11 +2647,15 @@ ssize_t ib_uverbs_create_flow(struct
>> ib_uverbs_file *file,
>> void *ib_spec;
>> int i;
>>
>> - if (out_len < sizeof(resp))
>> + if (ucore->outlen < sizeof(resp))
>> return -ENOSPC;
>>
>> - if (copy_from_user(&cmd, buf, sizeof(cmd)))
>> - return -EFAULT;
>> + err =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
>> + if (err)
>> + return err;
>> +
>> + ucore->inbuf +=izeof(cmd);
>> + ucore->inlen -=izeof(cmd);
>>
>> if (cmd.comp_mask)
>> return -EINVAL;
>> @@ -2671,7 +2667,7 @@ ssize_t ib_uverbs_create_flow(struct
>> ib_uverbs_file *file,
>> if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
>> return -EINVAL;
>>
>> - if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
>> + if (cmd.flow_attr.size > ucore->inlen ||
>> cmd.flow_attr.size >
>> (cmd.flow_attr.num_of_specs * sizeof(struct
>> ib_uverbs_flow_spec)))
>> return -EINVAL;
>> @@ -2683,11 +2679,10 @@ ssize_t ib_uverbs_create_flow(struct
>> ib_uverbs_file *file,
>> return -ENOMEM;
>>
>> memcpy(kern_flow_attr, &cmd.flow_attr,
>> sizeof(*kern_flow_attr));
>> - if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
>> - cmd.flow_attr.size)) {
>> - err =EFAULT;
>> + err =b_copy_from_udata(kern_flow_attr + 1, ucore,
>> + cmd.flow_attr.size);
>> + if (err)
>> goto err_free_attr;
>> - }
>> } else {
>> kern_flow_attr =cmd.flow_attr;
>> }
>> @@ -2755,11 +2750,10 @@ ssize_t ib_uverbs_create_flow(struct
>> ib_uverbs_file *file,
>> memset(&resp, 0, sizeof(resp));
>> resp.flow_handle =obj->id;
>>
>> - if (copy_to_user((void __user *)(unsigned long) cmd.response,
>> - &resp, sizeof(resp))) {
>> - err =EFAULT;
>> + err =b_copy_to_udata(ucore,
>> + &resp, sizeof(resp));
>> + if (err)
>> goto err_copy;
>> - }
>>
>> put_qp_read(qp);
>> mutex_lock(&file->mutex);
>> @@ -2772,7 +2766,7 @@ ssize_t ib_uverbs_create_flow(struct
>> ib_uverbs_file *file,
>> kfree(flow_attr);
>> if (cmd.flow_attr.num_of_specs)
>> kfree(kern_flow_attr);
>> - return in_len;
>> + return 0;
>> err_copy:
>> idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
>> destroy_flow:
>> @@ -2789,16 +2783,18 @@ err_free_attr:
>> return err;
>> }
>>
>> -ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
>> - const char __user *buf, int in_len,
>> - int out_len) {
>> +int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
>> + struct ib_udata *ucore,
>> + struct ib_udata *uhw)
>> +{
>> struct ib_uverbs_destroy_flow cmd;
>> struct ib_flow *flow_id;
>> struct ib_uobject *uobj;
>> int ret;
>>
>> - if (copy_from_user(&cmd, buf, sizeof(cmd)))
>> - return -EFAULT;
>> + ret =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
>> + if (ret)
>> + return ret;
>>
>> uobj =dr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
>> file->ucontext);
>> @@ -2820,7 +2816,7 @@ ssize_t ib_uverbs_destroy_flow(struct
>> ib_uverbs_file *file,
>>
>> put_uobj(uobj);
>>
>> - return ret ? ret : in_len;
>> + return ret ? ret : 0;
>> }
>>
>> static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
>> diff --git a/drivers/infiniband/core/uverbs_main.c
>> b/drivers/infiniband/core/uverbs_main.c
>> index 75ad86c..4f00654 100644
>> --- a/drivers/infiniband/core/uverbs_main.c
>> +++ b/drivers/infiniband/core/uverbs_main.c
>> @@ -115,8 +115,13 @@ static ssize_t (*uverbs_cmd_table[])(struct
>> ib_uverbs_file *file,
>> [IB_USER_VERBS_CMD_CLOSE_XRCD] =b_uverbs_close_xrcd,
>> [IB_USER_VERBS_CMD_CREATE_XSRQ] =b_uverbs_create_xsrq,
>> [IB_USER_VERBS_CMD_OPEN_QP] =b_uverbs_open_qp,
>> - [IB_USER_VERBS_CMD_CREATE_FLOW] =b_uverbs_create_flow,
>> - [IB_USER_VERBS_CMD_DESTROY_FLOW] =b_uverbs_destroy_flow
>> +};
>> +
>> +static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
>> + struct ib_udata *ucore,
>> + struct ib_udata *uhw) =
>> + [IB_USER_VERBS_EX_CMD_CREATE_FLOW] =b_uverbs_ex_create_flow,
>> + [IB_USER_VERBS_EX_CMD_DESTROY_FLOW] =b_uverbs_ex_destroy_flow
>> };
>>
>> static void ib_uverbs_add_one(struct ib_device *device);
>> @@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
>> const char __user *buf,
>> {
>> struct ib_uverbs_file *file =ilp->private_data;
>> struct ib_uverbs_cmd_hdr hdr;
>> + __u32 flags;
>>
>> if (count < sizeof hdr)
>> return -EINVAL;
>> @@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
>> *filp, const char __user *buf,
>> if (copy_from_user(&hdr, buf, sizeof hdr))
>> return -EFAULT;
>>
>> - if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
>> - !uverbs_cmd_table[hdr.command])
>> - return -EINVAL;
>> + flags =hdr.command &
>> + IB_USER_VERBS_CMD_FLAGS_MASK) >>
>> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>>
>> - if (!file->ucontext &&
>> - hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
>> - return -EINVAL;
>> + if (!flags) {
>> + __u32 command;
>>
>> - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>> hdr.command)))
>> - return -ENOSYS;
>> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
>> +
>> IB_USER_VERBS_CMD_COMMAND_MASK))
>> + return -EINVAL;
>>
>> - if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
>> - struct ib_uverbs_cmd_hdr_ex hdr_ex;
>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>
>> - if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
>> - return -EFAULT;
>> + if (command >=RRAY_SIZE(uverbs_cmd_table) ||
>> + !uverbs_cmd_table[command])
>> + return -EINVAL;
>>
>> - if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4)
>> !=ount)
>> + if (!file->ucontext &&
>> + command !=B_USER_VERBS_CMD_GET_CONTEXT)
>> return -EINVAL;
>>
>> - return uverbs_cmd_table[hdr.command](file,
>> - buf +
>> sizeof(hdr_ex),
>> - (hdr_ex.in_words +
>> -
>> hdr_ex.provider_in_words) * 4,
>> - (hdr_ex.out_words +
>> -
>> hdr_ex.provider_out_words) * 4);
>> - } else {
>> + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>> command)))
>> + return -ENOSYS;
>> +
>> if (hdr.in_words * 4 !=ount)
>> return -EINVAL;
>>
>> - return uverbs_cmd_table[hdr.command](file,
>> - buf + sizeof(hdr),
>> - hdr.in_words * 4,
>> - hdr.out_words * 4);
>> + return uverbs_cmd_table[command](file,
>> + buf + sizeof(hdr),
>> + hdr.in_words * 4,
>> + hdr.out_words * 4);
>> +
>> + } else if (flags =IB_USER_VERBS_CMD_FLAG_EXTENDED) {
>> + __u32 command;
>> +
>> + struct ib_uverbs_ex_cmd_hdr ex_hdr;
>> + struct ib_udata ucore;
>> + struct ib_udata uhw;
>> + int err;
>> +
>> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
>> +
>> IB_USER_VERBS_CMD_COMMAND_MASK))
>> + return -EINVAL;
>> +
>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>> +
>> + if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
>> + !uverbs_ex_cmd_table[command])
>> + return -ENOSYS;
>> +
>> + if (!file->ucontext)
>> + return -EINVAL;
>> +
>> + if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull
>> << command)))
>> + return -ENOSYS;
>> +
>> + if (count < (sizeof(hdr) + sizeof(ex_hdr)))
>> + return -EINVAL;
>> +
>> + if (copy_from_user(&ex_hdr, buf + sizeof(hdr),
>> sizeof(ex_hdr)))
>> + return -EFAULT;
>> +
>> + count -=izeof(hdr) + sizeof(ex_hdr);
A small issue: count is reduced here and the write function returns the
reduced size.
I think we should return the amount of data the user wrote and not to
subtract the headers from the returned size.
>> + buf +=izeof(hdr) + sizeof(ex_hdr);
>> +
>> + if ((hdr.in_words + ex_hdr.provider_in_words) * 8 !=ount)
>> + return -EINVAL;
>> +
>> + if (ex_hdr.response) {
>> + if (!hdr.out_words && !ex_hdr.provider_out_words)
>> + return -EINVAL;
>> + } else {
>> + if (hdr.out_words || ex_hdr.provider_out_words)
>> + return -EINVAL;
>> + }
>> +
>> + INIT_UDATA(&ucore,
>> + (hdr.in_words) ? buf : 0,
>> + (unsigned long)ex_hdr.response,
>> + hdr.in_words * 8,
>> + hdr.out_words * 8);
>> +
>> + INIT_UDATA(&uhw,
>> + (ex_hdr.provider_in_words) ? buf +
>> ucore.inlen : 0,
>> + (ex_hdr.provider_out_words) ? (unsigned
>> long)ex_hdr.response + ucore.outlen : 0,
>> + ex_hdr.provider_in_words * 8,
>> + ex_hdr.provider_out_words * 8);
>> +
>> + err =verbs_ex_cmd_table[command](file,
>> + &ucore,
>> + &uhw);
>> +
>> + if (err)
>> + return err;
>> +
>> + return count;
====================^
>> }
>> +
>> + return -ENOSYS;
>> }
>>
>> static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct
>> *vma)
>> diff --git a/drivers/infiniband/hw/mlx4/main.c
>> b/drivers/infiniband/hw/mlx4/main.c
>> index d6c5a73..1aad9b3 100644
>> --- a/drivers/infiniband/hw/mlx4/main.c
>> +++ b/drivers/infiniband/hw/mlx4/main.c
>> @@ -1691,9 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
>> ibdev->ib_dev.create_flow =lx4_ib_create_flow;
>> ibdev->ib_dev.destroy_flow =lx4_ib_destroy_flow;
>>
>> - ibdev->ib_dev.uverbs_cmd_mask
>> |- (1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |
>> - (1ull << IB_USER_VERBS_CMD_DESTROY_FLOW);
>> + ibdev->ib_dev.uverbs_ex_cmd_mask
>> |+ (1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |
>> + (1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
>> }
>>
>> mlx4_ib_alloc_eqs(dev, ibdev);
>> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
>> index e393171..a06fc12 100644
>> --- a/include/rdma/ib_verbs.h
>> +++ b/include/rdma/ib_verbs.h
>> @@ -1436,6 +1436,7 @@ struct ib_device {
>>
>> int uverbs_abi_ver;
>> u64 uverbs_cmd_mask;
>> + u64 uverbs_ex_cmd_mask;
>>
>> char node_desc[64];
>> __be64 node_guid;
>> diff --git a/include/uapi/rdma/ib_user_verbs.h
>> b/include/uapi/rdma/ib_user_verbs.h
>> index 93577fc..cbfdd4c 100644
>> --- a/include/uapi/rdma/ib_user_verbs.h
>> +++ b/include/uapi/rdma/ib_user_verbs.h
>> @@ -87,8 +87,11 @@ enum {
>> IB_USER_VERBS_CMD_CLOSE_XRCD,
>> IB_USER_VERBS_CMD_CREATE_XSRQ,
>> IB_USER_VERBS_CMD_OPEN_QP,
>> - IB_USER_VERBS_CMD_CREATE_FLOW =B_USER_VERBS_CMD_THRESHOLD,
>> - IB_USER_VERBS_CMD_DESTROY_FLOW
>> +};
>> +
>> +enum {
>> + IB_USER_VERBS_EX_CMD_CREATE_FLOW =B_USER_VERBS_CMD_THRESHOLD,
>> + IB_USER_VERBS_EX_CMD_DESTROY_FLOW
>> };
>
> I think B_USER_VERBS_CMD_THRESHOLD could be removed.
>
>>
>> /*
>> @@ -120,16 +123,20 @@ struct ib_uverbs_comp_event_desc {
>> * the rest of the command struct based on these value.
>> */
>>
>> +#define IB_USER_VERBS_CMD_COMMAND_MASK 0xff
>> +#define IB_USER_VERBS_CMD_FLAGS_MASK 0xff000000u
>> +#define IB_USER_VERBS_CMD_FLAGS_SHIFT 24
>> +
>> +#define IB_USER_VERBS_CMD_FLAG_EXTENDED 0x80
>> +
>> struct ib_uverbs_cmd_hdr {
>> __u32 command;
>> __u16 in_words;
>> __u16 out_words;
>> };
>>
>> -struct ib_uverbs_cmd_hdr_ex {
>> - __u32 command;
>> - __u16 in_words;
>> - __u16 out_words;
>> +struct ib_uverbs_ex_cmd_hdr {
>> + __u64 response;
>> __u16 provider_in_words;
>> __u16 provider_out_words;
>> __u32 cmd_hdr_reserved;
>> @@ -777,8 +784,6 @@ struct ib_uverbs_flow_attr {
>>
>> struct ib_uverbs_create_flow {
>> __u32 comp_mask;
>> - __u32 reserved;
>> - __u64 response;
>> __u32 qp_handle;
>> struct ib_uverbs_flow_attr flow_attr;
>> };
>> --
>> 1.8.3.1
>>
>
> This infrastructure (as well as the original one) doesn't deal with the
> case we discussed about in the previous patches. When a command contains
> a structure that can be extended, we don't have a clean nice way of
> doing that. Though, because it's a limitation of the original design and
> this patch cleans up the functionality of the original infrastructure,
> I'm in favor of merging it.
>
> Matan
Regarding the disable of flow steering commands, we could just remove
the flow steering uverbs from the commands array, but then we'll have to
count on the compiler to do the required optimizations.
Matan
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands
[not found] ` <525FF724.5050601-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-10-17 15:05 ` Yann Droneaud
[not found] ` <f45595bcb4399ea2cb81d99a914dd9a0-zgzEX58YAwA@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-17 15:05 UTC (permalink / raw)
To: Matan Barak
Cc: Roland Dreier, Roland Dreier, Or Gerlitz,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, Igor Ivanov
Hi,
Le 17.10.2013 16:41, Matan Barak a écrit :
> On 14/10/2013 6:38 PM, Matan Barak wrote:
>> On 11/10/2013 8:57 PM, Yann Droneaud wrote:
>>> Commit 400dbc9 added an infrastructure for extensible uverbs
>>> commands while later commit 436f2ad exported
>>> ib_create_flow()/ib_destroy_flow()
>>> functions using this new infrastructure.
>>>
>>> diff --git a/drivers/infiniband/core/uverbs_main.c
>>> b/drivers/infiniband/core/uverbs_main.c
>>> index 75ad86c..4f00654 100644
>>> --- a/drivers/infiniband/core/uverbs_main.c
>>> +++ b/drivers/infiniband/core/uverbs_main.c
>>> @@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
>>> @@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
>>> *filp, const char __user *buf,
>>> if (copy_from_user(&hdr, buf, sizeof hdr))
>>> return -EFAULT;
>>>
>>> - if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
>>> - !uverbs_cmd_table[hdr.command])
>>> - return -EINVAL;
>>> + flags =hdr.command &
>>> + IB_USER_VERBS_CMD_FLAGS_MASK) >>
>>> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>>>
>>> - if (!file->ucontext &&
>>> - hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
>>> - return -EINVAL;
>>> + if (!flags) {
>>> + __u32 command;
>>>
>>> - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>>> hdr.command)))
>>> - return -ENOSYS;
>>> + if (hdr.command &
>>> ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
>>> +
>>> IB_USER_VERBS_CMD_COMMAND_MASK))
>>> + return -EINVAL;
>>>
>>> - if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
>>> - struct ib_uverbs_cmd_hdr_ex hdr_ex;
>>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>>
>>> - if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
>>> - return -EFAULT;
>>> + if (command >=RRAY_SIZE(uverbs_cmd_table) ||
>>> + !uverbs_cmd_table[command])
>>> + return -EINVAL;
>>>
>>> - if (((hdr_ex.in_words + hdr_ex.provider_in_words) *
>>> 4)
>>> !=ount)
>>> + if (!file->ucontext &&
>>> + command !=B_USER_VERBS_CMD_GET_CONTEXT)
>>> return -EINVAL;
>>>
>>> - return uverbs_cmd_table[hdr.command](file,
>>> - buf +
>>> sizeof(hdr_ex),
>>> - (hdr_ex.in_words
>>> +
>>> -
>>> hdr_ex.provider_in_words) * 4,
>>> -
>>> (hdr_ex.out_words +
>>> -
>>> hdr_ex.provider_out_words) * 4);
>>> - } else {
>>> + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull
>>> <<
>>> command)))
>>> + return -ENOSYS;
>>> +
>>> if (hdr.in_words * 4 !=ount)
>>> return -EINVAL;
>>>
>>> - return uverbs_cmd_table[hdr.command](file,
>>> - buf +
>>> sizeof(hdr),
>>> - hdr.in_words *
>>> 4,
>>> - hdr.out_words *
>>> 4);
>>> + return uverbs_cmd_table[command](file,
>>> + buf + sizeof(hdr),
>>> + hdr.in_words * 4,
>>> + hdr.out_words * 4);
>>> +
>>> + } else if (flags =IB_USER_VERBS_CMD_FLAG_EXTENDED) {
>>> + __u32 command;
>>> +
>>> + struct ib_uverbs_ex_cmd_hdr ex_hdr;
>>> + struct ib_udata ucore;
>>> + struct ib_udata uhw;
>>> + int err;
>>> +
>>> + if (hdr.command &
>>> ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
>>> +
>>> IB_USER_VERBS_CMD_COMMAND_MASK))
>>> + return -EINVAL;
>>> +
>>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>> +
>>> + if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
>>> + !uverbs_ex_cmd_table[command])
>>> + return -ENOSYS;
>>> +
>>> + if (!file->ucontext)
>>> + return -EINVAL;
>>> +
>>> + if (!(file->device->ib_dev->uverbs_ex_cmd_mask &
>>> (1ull
>>> << command)))
>>> + return -ENOSYS;
>>> +
>>> + if (count < (sizeof(hdr) + sizeof(ex_hdr)))
>>> + return -EINVAL;
>>> +
>>> + if (copy_from_user(&ex_hdr, buf + sizeof(hdr),
>>> sizeof(ex_hdr)))
>>> + return -EFAULT;
>>> +
>>> + count -=izeof(hdr) + sizeof(ex_hdr);
>
> A small issue: count is reduced here and the write function returns
> the reduced size.
> I think we should return the amount of data the user wrote and not to
> subtract the headers from the returned size.
>
Good catch.
>>> +
>>> + return count;
>
> ====================^
>
>>> diff --git a/include/uapi/rdma/ib_user_verbs.h
>>> b/include/uapi/rdma/ib_user_verbs.h
>>> index 93577fc..cbfdd4c 100644
>>> --- a/include/uapi/rdma/ib_user_verbs.h
>>> +++ b/include/uapi/rdma/ib_user_verbs.h
>>> @@ -87,8 +87,11 @@ enum {
>>> IB_USER_VERBS_CMD_CLOSE_XRCD,
>>> IB_USER_VERBS_CMD_CREATE_XSRQ,
>>> IB_USER_VERBS_CMD_OPEN_QP,
>>> - IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
>>> - IB_USER_VERBS_CMD_DESTROY_FLOW
>>> +};
>>> +
>>> +enum {
>>> + IB_USER_VERBS_EX_CMD_CREATE_FLOW =
>>> IB_USER_VERBS_CMD_THRESHOLD,
>>> + IB_USER_VERBS_EX_CMD_DESTROY_FLOW
>>> };
>>
>> I think B_USER_VERBS_CMD_THRESHOLD could be removed.
>>
If it's removed, CREATE_FLOW will be the first command.
I thought it could be interesting to keep room for the "legacy"
commands if they're going to be supported as extended commands
in the future.
Regards.
--
Yann Droneaud
OPTEYA
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <525FABA2.6000507-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-10-17 15:29 ` Yann Droneaud
[not found] ` <80c62f070af55a176923315b3fbb7b0d-zgzEX58YAwA@public.gmane.org>
0 siblings, 1 reply; 31+ messages in thread
From: Yann Droneaud @ 2013-10-17 15:29 UTC (permalink / raw)
To: Or Gerlitz
Cc: Roland Dreier, Tzahi Oved, Matan Barak,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA
Hi Or,
Le 17.10.2013 11:19, Or Gerlitz a écrit :
> On 16/10/2013 03:04, Roland Dreier wrote:
>> On Tue, Oct 15, 2013 at 2:34 PM, Or Gerlitz <or.gerlitz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> wrote:
>>> Roland, after applying a small fix which was sent to you by Matan on
>>> Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be
>>> surely squashed into one small patch), things are working OK. Yann is
>>> suggesting to enhance them to work and look even better and for some
>>> reasons (life) he didn't get to do that on the 3-4 months the patches
>>> spent on the list and went through review of Sean/Jason/Shawn and you
>>> (RD). I don't see how/why you use the term half-baked here.
>> Unless I'm misunderstanding, Yann's suggestion for enhancement is not
>> user/kernel ABI compatible with what is in the tree now. Given that
>> we should really try to find an ABI that lasts for a long time, and
>> that we already have a proposal to improve things, shouldn't we hold
>> off on freezing the new ABI for one more kernel cycle?
>>
>>
>
> Roland, Yann, the uverbs extensions + flow-steering patches spent
> many months on the list, once the review started
> we did quick fixes for all the comments and made it for 3.12-rc1 which
> was delay or two kernel cycles vs. what
> could happen if the feedback was on time (*).
>
"On time" is a matter of schedule, and since we don't (all) work
for the same employer, we don't have exact matching priorities.
But more than an agenda, we share the principles of code quality,
maintainability, coherency, usability.
My purpose was not to delay the wide availability of the flow steering
feature. I'm just lazily maintaining a "bound checking" patchset that
got
"hit" by the flow steering functions when they entered in -next.
I was not very impressed by the new interface and provided some late
feedback.
Since then, I have more time (spare time between real work and kids) and
focus more on the patches submitted on the list.
But as spare time (kernel) developer, I'm not able to provide as much
valuable contributions as full time kernel developer.
> Indeed since we do want to have ABI that lasts for long, deferring the
> uverbs support for extensions to 3.13 along with
> flow-steering support to user space is something we can live with,
> conditioned on commitment of Yann to quickly submit
> his fixed patches that addresses the issues Matan raised and of you
> (RD) to pick them once Yann and Matan agree
> its done into for-next so they are safe for 3.13
>
I can only agree to provide fixed patches for 3.13.
If I could find time and inspiration, I will try to provide
a novel scheme for command expansion that could fulfill the
what was expressed by Matan. But perhaps someone would come with
a better idea.
> Just to double check, by no means we are talking on reverting the
> whole series, flow-steering will be in
> 3.12 in the IB core and mlx4_ib
>
I believe it will disable by #if 0 / #endif.
It could be protected by #ifdef
CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
instead, but I don't know if it's acceptable practice for such piece of
code
that should be exposed in its current state.
Regards.
--
Yann Droneaud
OPTEYA
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 3/9] IB/core: Don't include command header size in uverbs create_flow
[not found] ` <80c62f070af55a176923315b3fbb7b0d-zgzEX58YAwA@public.gmane.org>
@ 2013-10-21 16:46 ` Roland Dreier
0 siblings, 0 replies; 31+ messages in thread
From: Roland Dreier @ 2013-10-21 16:46 UTC (permalink / raw)
To: Yann Droneaud
Cc: Or Gerlitz, Tzahi Oved, Matan Barak,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA
On Thu, Oct 17, 2013 at 8:29 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
>> Just to double check, by no means we are talking on reverting the
>> whole series, flow-steering will be in
>> 3.12 in the IB core and mlx4_ib
Correct, just hiding the unstable userspace ABI for now.
> I believe it will disable by #if 0 / #endif.
> It could be protected by #ifdef
> CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
> instead, but I don't know if it's acceptable practice for such piece of code
> that should be exposed in its current state.
I like the Kconfig option -- I just made a patch that does that (and
made the option depend on STAGING), I'll post it for comments.
- R.
--
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
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands
[not found] ` <f45595bcb4399ea2cb81d99a914dd9a0-zgzEX58YAwA@public.gmane.org>
@ 2013-10-24 0:34 ` Or Gerlitz
0 siblings, 0 replies; 31+ messages in thread
From: Or Gerlitz @ 2013-10-24 0:34 UTC (permalink / raw)
To: Yann Droneaud
Cc: Matan Barak, Roland Dreier, Roland Dreier, Or Gerlitz, linux-rdma,
Igor Ivanov
On Thu, Oct 17, 2013 at 6:05 PM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> Hi,
>
> Le 17.10.2013 16:41, Matan Barak a écrit :
>
>> On 14/10/2013 6:38 PM, Matan Barak wrote:
>>>
>>> On 11/10/2013 8:57 PM, Yann Droneaud wrote:
>>>>
>>>> Commit 400dbc9 added an infrastructure for extensible uverbs
>>>> commands while later commit 436f2ad exported
>>>> ib_create_flow()/ib_destroy_flow()
>>>> functions using this new infrastructure.
>>>>
>
>>>> diff --git a/drivers/infiniband/core/uverbs_main.c
>>>> b/drivers/infiniband/core/uverbs_main.c
>>>> index 75ad86c..4f00654 100644
>>>> --- a/drivers/infiniband/core/uverbs_main.c
>>>> +++ b/drivers/infiniband/core/uverbs_main.c
>>>> @@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
>>>> @@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
>>>> *filp, const char __user *buf,
>>>> if (copy_from_user(&hdr, buf, sizeof hdr))
>>>> return -EFAULT;
>>>>
>>>> - if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
>>>> - !uverbs_cmd_table[hdr.command])
>>>> - return -EINVAL;
>>>> + flags =hdr.command &
>>>> + IB_USER_VERBS_CMD_FLAGS_MASK) >>
>>>> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>>>>
>>>> - if (!file->ucontext &&
>>>> - hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
>>>> - return -EINVAL;
>>>> + if (!flags) {
>>>> + __u32 command;
>>>>
>>>> - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>>>> hdr.command)))
>>>> - return -ENOSYS;
>>>> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK
>>>> |
>>>> +
>>>> IB_USER_VERBS_CMD_COMMAND_MASK))
>>>> + return -EINVAL;
>>>>
>>>> - if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
>>>> - struct ib_uverbs_cmd_hdr_ex hdr_ex;
>>>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>>>
>>>> - if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
>>>> - return -EFAULT;
>>>> + if (command >=RRAY_SIZE(uverbs_cmd_table) ||
>>>> + !uverbs_cmd_table[command])
>>>> + return -EINVAL;
>>>>
>>>> - if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4)
>>>> !=ount)
>>>> + if (!file->ucontext &&
>>>> + command !=B_USER_VERBS_CMD_GET_CONTEXT)
>>>> return -EINVAL;
>>>>
>>>> - return uverbs_cmd_table[hdr.command](file,
>>>> - buf +
>>>> sizeof(hdr_ex),
>>>> - (hdr_ex.in_words +
>>>> -
>>>> hdr_ex.provider_in_words) * 4,
>>>> - (hdr_ex.out_words +
>>>> -
>>>> hdr_ex.provider_out_words) * 4);
>>>> - } else {
>>>> + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>>>> command)))
>>>> + return -ENOSYS;
>>>> +
>>>> if (hdr.in_words * 4 !=ount)
>>>> return -EINVAL;
>>>>
>>>> - return uverbs_cmd_table[hdr.command](file,
>>>> - buf + sizeof(hdr),
>>>> - hdr.in_words * 4,
>>>> - hdr.out_words * 4);
>>>> + return uverbs_cmd_table[command](file,
>>>> + buf + sizeof(hdr),
>>>> + hdr.in_words * 4,
>>>> + hdr.out_words * 4);
>>>> +
>>>> + } else if (flags =IB_USER_VERBS_CMD_FLAG_EXTENDED) {
>>>> + __u32 command;
>>>> +
>>>> + struct ib_uverbs_ex_cmd_hdr ex_hdr;
>>>> + struct ib_udata ucore;
>>>> + struct ib_udata uhw;
>>>> + int err;
>>>> +
>>>> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK
>>>> |
>>>> +
>>>> IB_USER_VERBS_CMD_COMMAND_MASK))
>>>> + return -EINVAL;
>>>> +
>>>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>>> +
>>>> + if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
>>>> + !uverbs_ex_cmd_table[command])
>>>> + return -ENOSYS;
>>>> +
>>>> + if (!file->ucontext)
>>>> + return -EINVAL;
>>>> +
>>>> + if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull
>>>> << command)))
>>>> + return -ENOSYS;
>>>> +
>>>> + if (count < (sizeof(hdr) + sizeof(ex_hdr)))
>>>> + return -EINVAL;
>>>> +
>>>> + if (copy_from_user(&ex_hdr, buf + sizeof(hdr),
>>>> sizeof(ex_hdr)))
>>>> + return -EFAULT;
>>>> +
>>>> + count -=izeof(hdr) + sizeof(ex_hdr);
>>
>>
>> A small issue: count is reduced here and the write function returns
>> the reduced size.
>> I think we should return the amount of data the user wrote and not to
>> subtract the headers from the returned size.
>>
>
> Good catch.
Yann, the 3.13 merge window is coming closer, can you please post V2
of this series with fixes to the few
issues Matan was pointing on and possibly one squash @ the beginning
of the series, if this is possible.
>
>
>>>> +
>>>> + return count;
>>
>>
>> ====================^
>>
>
>
>>>> diff --git a/include/uapi/rdma/ib_user_verbs.h
>>>> b/include/uapi/rdma/ib_user_verbs.h
>>>> index 93577fc..cbfdd4c 100644
>>>> --- a/include/uapi/rdma/ib_user_verbs.h
>>>> +++ b/include/uapi/rdma/ib_user_verbs.h
>>>> @@ -87,8 +87,11 @@ enum {
>>>> IB_USER_VERBS_CMD_CLOSE_XRCD,
>>>> IB_USER_VERBS_CMD_CREATE_XSRQ,
>>>> IB_USER_VERBS_CMD_OPEN_QP,
>>>> - IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
>>>>
>>>> - IB_USER_VERBS_CMD_DESTROY_FLOW
>>>> +};
>>>> +
>>>> +enum {
>>>> + IB_USER_VERBS_EX_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
>>>> + IB_USER_VERBS_EX_CMD_DESTROY_FLOW
>>>> };
>>>
>>>
>>> I think B_USER_VERBS_CMD_THRESHOLD could be removed.
>>>
>
> If it's removed, CREATE_FLOW will be the first command.
>
> I thought it could be interesting to keep room for the "legacy"
> commands if they're going to be supported as extended commands
> in the future.
>
> Regards.
>
> --
> Yann Droneaud
> OPTEYA
>
>
> --
> 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
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2013-10-24 0:34 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-11 17:18 [PATCH 0/9] IB/core: batch of fix against create/destroy_flow uverbs for v3.12 Yann Droneaud
[not found] ` <cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-11 17:18 ` [PATCH 1/9] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow Yann Droneaud
2013-10-11 17:18 ` [PATCH 2/9] IB/core: Includes flow attribute size in uverbs create_flow Yann Droneaud
[not found] ` <a83e37e225cd16e5a556b35dcadc7a1234519c2a.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 11:01 ` Or Gerlitz
[not found] ` <525BCF21.3090706-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-14 12:22 ` Yann Droneaud
2013-10-14 15:13 ` Matan Barak
2013-10-11 17:18 ` [PATCH 3/9] IB/core: Don't include command header " Yann Droneaud
[not found] ` <ca92c22d0aea21d559d57e6152cb313729a11274.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 15:21 ` Matan Barak
[not found] ` <525C0BF2.2060201-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-14 15:46 ` Yann Droneaud
[not found] ` <72b95284386dc57dfa1c7c883b4f45af-zgzEX58YAwA@public.gmane.org>
2013-10-14 18:19 ` Roland Dreier
[not found] ` <CAL1RGDWarJPu_6u5oaL6NgZZJXBuB09vTi9xhxHALfiY_W9Sgg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-10-15 14:08 ` Yann Droneaud
[not found] ` <122f1a29d6fe45449a6a153a3a9ba8b1-zgzEX58YAwA@public.gmane.org>
2013-10-15 16:44 ` Roland Dreier
[not found] ` <CAL1RGDXP3+X=go4GiW_SsvSvC_VS-B7uacJN0u5wVtU3NWfvQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-10-15 17:07 ` Yann Droneaud
2013-10-15 21:34 ` Or Gerlitz
[not found] ` <CAJZOPZLr41MGb7qPHcFVQxNcwwAB8i4vnGQhEcu0oZ-BOiMBcg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-10-16 0:04 ` Roland Dreier
[not found] ` <CAL1RGDW7KwSOmmsRxDokUHR-Mh1OzaQvDege5Rq7JsSU8AgEHQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2013-10-17 9:19 ` Or Gerlitz
[not found] ` <525FABA2.6000507-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-17 15:29 ` Yann Droneaud
[not found] ` <80c62f070af55a176923315b3fbb7b0d-zgzEX58YAwA@public.gmane.org>
2013-10-21 16:46 ` Roland Dreier
2013-10-11 17:18 ` [PATCH 4/9] IB/core: Rename 'flow' structs to match other uverbs structs Yann Droneaud
2013-10-11 17:18 ` [PATCH 5/9] IB/core: Makes uverbs flow structure using names more similar to verbs ones Yann Droneaud
2013-10-11 17:18 ` [PATCH 6/9] IB/core: Uses a common header for uverbs flow_specs Yann Droneaud
2013-10-11 17:18 ` [PATCH 7/9] IB/core: Remove ib_uverbs_flow_spec structure from userspace Yann Droneaud
[not found] ` <f8673f302536d503cdeef005a67f4531706ae578.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 15:26 ` Matan Barak
2013-10-11 17:18 ` [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands Yann Droneaud
[not found] ` <0b84e20b204eb0fc67db9ca9106d6bf753a12ae3.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 15:38 ` Matan Barak
[not found] ` <525C0FF2.5040905-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-17 14:41 ` Matan Barak
[not found] ` <525FF724.5050601-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-17 15:05 ` Yann Droneaud
[not found] ` <f45595bcb4399ea2cb81d99a914dd9a0-zgzEX58YAwA@public.gmane.org>
2013-10-24 0:34 ` Or Gerlitz
2013-10-11 17:18 ` [PATCH 9/9] IB/core: extended command: move comp_mask to the extended header Yann Droneaud
[not found] ` <40175eda10d670d098204da6aa4c327a0171ae5f.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
2013-10-14 15:44 ` Matan Barak
2013-10-14 15:36 ` [PATCH 0/9] IB/core: batch of fix against create/destroy_flow uverbs for v3.12 Or Gerlitz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox