| Message ID | caeb36c2271945405dae54502526f5de02cd63e7.1519688087.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Feb 26, 2018 at 03:22:14PM -0800, Steve Wise wrote: > Create a common dumpit function that can be used by all common resource > types. This reduces code replication and simplifies the code as we add > more resource types. > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > --- > drivers/infiniband/core/nldev.c | 61 +++++++++++++++++++++++++++++------------ > 1 file changed, 44 insertions(+), 17 deletions(-) > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 5326a68..4f1cfa6 100644 > --- a/drivers/infiniband/core/nldev.c > +++ b/drivers/infiniband/core/nldev.c > @@ -212,10 +212,10 @@ static int fill_res_info(struct sk_buff *msg, struct ib_device *device) > return ret; > } > > -static int fill_res_qp_entry(struct sk_buff *msg, > - struct ib_qp *qp, uint32_t port) > +static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, > + struct rdma_restrack_entry *res, uint32_t port) > { > - struct rdma_restrack_entry *res = &qp->res; > + struct ib_qp *qp = container_of(res, struct ib_qp, res); > struct ib_qp_init_attr qp_init_attr; > struct nlattr *entry_attr; > struct ib_qp_attr qp_attr; > @@ -558,8 +558,17 @@ static int nldev_res_get_dumpit(struct sk_buff *skb, > return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb); > } > > -static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > - struct netlink_callback *cb) > +struct nldev_fill_res_entry { > + int (*fill_res_func)(struct sk_buff *msg, struct netlink_callback *cb, > + struct rdma_restrack_entry *res, u32 port); > + enum rdma_restrack_type res_type; > + enum rdma_nldev_attr nldev_attr; > + enum rdma_nldev_command nldev_cmd; > +}; > + > +static int res_get_common_dumpit(struct sk_buff *skb, > + struct netlink_callback *cb, > + struct nldev_fill_res_entry *fill_entry) > { > struct nlattr *tb[RDMA_NLDEV_ATTR_MAX]; > struct rdma_restrack_entry *res; > @@ -567,9 +576,9 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > struct nlattr *table_attr; > struct ib_device *device; > int start = cb->args[0]; > - struct ib_qp *qp = NULL; > struct nlmsghdr *nlh; > u32 index, port = 0; > + bool filled = false; > > err = nlmsg_parse(cb->nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, > nldev_policy, NULL); > @@ -601,7 +610,7 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > } > > nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, > - RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_QP_GET), > + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, fill_entry->nldev_cmd), > 0, NLM_F_MULTI); > > if (fill_nldev_handle(skb, device)) { > @@ -609,24 +618,27 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > goto err; > } > > - table_attr = nla_nest_start(skb, RDMA_NLDEV_ATTR_RES_QP); > + table_attr = nla_nest_start(skb, fill_entry->nldev_attr); > if (!table_attr) { > ret = -EMSGSIZE; > goto err; > } > > down_read(&device->res.rwsem); > - hash_for_each_possible(device->res.hash, res, node, RDMA_RESTRACK_QP) { > + hash_for_each_possible(device->res.hash, res, node, > + fill_entry->res_type) { > if (idx < start) > goto next; > > if ((rdma_is_kernel_res(res) && > task_active_pid_ns(current) != &init_pid_ns) || > - (!rdma_is_kernel_res(res) && > - task_active_pid_ns(current) != task_active_pid_ns(res->task))) > + (!rdma_is_kernel_res(res) && task_active_pid_ns(current) != > + task_active_pid_ns(res->task))) > /* > - * 1. Kernel QPs should be visible in init namspace only > - * 2. Present only QPs visible in the current namespace > + * 1. Kern resources should be visible in init > + * namspace only > + * 2. Present only resources visible in the current > + * namespace > */ > goto next; > > @@ -638,10 +650,10 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > */ > goto next; > > - qp = container_of(res, struct ib_qp, res); > + filled = true; > > up_read(&device->res.rwsem); > - ret = fill_res_qp_entry(skb, qp, port); > + ret = fill_entry->fill_res_func(skb, cb, res, port); > down_read(&device->res.rwsem); > /* > * Return resource back, but it won't be released till > @@ -667,10 +679,10 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > cb->args[0] = idx; > > /* > - * No more QPs to fill, cancel the message and > + * No more entries to fill, cancel the message and > * return 0 to mark end of dumpit. > */ > - if (!qp) > + if (!filled) > goto err; > > put_device(&device->dev); > @@ -688,6 +700,21 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > return ret; > } > > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > + [RDMA_RESTRACK_QP] = { > + .fill_res_func = fill_res_qp_entry, > + .res_type = RDMA_RESTRACK_QP, > + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, > + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, > + }, > +}; Steve, I imagined such function callback code a little bit simpler than it is now: nldev_res_get_XXX_dumpit { initialization checks ->callback(skb, RDMA_RESTRACK_XXX) close/error handling } It will simplify the callback table a bit. > + > +static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > + struct netlink_callback *cb) > +{ > + return res_get_common_dumpit(skb, cb, &fill_entries[RDMA_RESTRACK_QP]); > +} > + > static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = { > [RDMA_NLDEV_CMD_GET] = { > .doit = nldev_get_doit, > -- > 1.8.3.1 >
> > On Mon, Feb 26, 2018 at 03:22:14PM -0800, Steve Wise wrote: > > Create a common dumpit function that can be used by all common > resource > > types. This reduces code replication and simplifies the code as we add > > more resource types. > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > --- > > drivers/infiniband/core/nldev.c | 61 +++++++++++++++++++++++++++++---- > -------- > > 1 file changed, 44 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/infiniband/core/nldev.c > b/drivers/infiniband/core/nldev.c > > index 5326a68..4f1cfa6 100644 > > --- a/drivers/infiniband/core/nldev.c > > +++ b/drivers/infiniband/core/nldev.c > > @@ -212,10 +212,10 @@ static int fill_res_info(struct sk_buff *msg, struct > ib_device *device) > > return ret; > > } > > > > -static int fill_res_qp_entry(struct sk_buff *msg, > > - struct ib_qp *qp, uint32_t port) > > +static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, > > + struct rdma_restrack_entry *res, uint32_t port) > > { > > - struct rdma_restrack_entry *res = &qp->res; > > + struct ib_qp *qp = container_of(res, struct ib_qp, res); > > struct ib_qp_init_attr qp_init_attr; > > struct nlattr *entry_attr; > > struct ib_qp_attr qp_attr; > > @@ -558,8 +558,17 @@ static int nldev_res_get_dumpit(struct sk_buff > *skb, > > return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb); > > } > > > > -static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > > - struct netlink_callback *cb) > > +struct nldev_fill_res_entry { > > + int (*fill_res_func)(struct sk_buff *msg, struct netlink_callback *cb, > > + struct rdma_restrack_entry *res, u32 port); > > + enum rdma_restrack_type res_type; > > + enum rdma_nldev_attr nldev_attr; > > + enum rdma_nldev_command nldev_cmd; > > +}; > > + > > +static int res_get_common_dumpit(struct sk_buff *skb, > > + struct netlink_callback *cb, > > + struct nldev_fill_res_entry *fill_entry) > > { > > struct nlattr *tb[RDMA_NLDEV_ATTR_MAX]; > > struct rdma_restrack_entry *res; > > @@ -567,9 +576,9 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > *skb, > > struct nlattr *table_attr; > > struct ib_device *device; > > int start = cb->args[0]; > > - struct ib_qp *qp = NULL; > > struct nlmsghdr *nlh; > > u32 index, port = 0; > > + bool filled = false; > > > > err = nlmsg_parse(cb->nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, > > nldev_policy, NULL); > > @@ -601,7 +610,7 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > *skb, > > } > > > > nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh- > >nlmsg_seq, > > - RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, > RDMA_NLDEV_CMD_RES_QP_GET), > > + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, fill_entry- > >nldev_cmd), > > 0, NLM_F_MULTI); > > > > if (fill_nldev_handle(skb, device)) { > > @@ -609,24 +618,27 @@ static int nldev_res_get_qp_dumpit(struct > sk_buff *skb, > > goto err; > > } > > > > - table_attr = nla_nest_start(skb, RDMA_NLDEV_ATTR_RES_QP); > > + table_attr = nla_nest_start(skb, fill_entry->nldev_attr); > > if (!table_attr) { > > ret = -EMSGSIZE; > > goto err; > > } > > > > down_read(&device->res.rwsem); > > - hash_for_each_possible(device->res.hash, res, node, > RDMA_RESTRACK_QP) { > > + hash_for_each_possible(device->res.hash, res, node, > > + fill_entry->res_type) { > > if (idx < start) > > goto next; > > > > if ((rdma_is_kernel_res(res) && > > task_active_pid_ns(current) != &init_pid_ns) || > > - (!rdma_is_kernel_res(res) && > > - task_active_pid_ns(current) != task_active_pid_ns(res- > >task))) > > + (!rdma_is_kernel_res(res) && task_active_pid_ns(current) > != > > + task_active_pid_ns(res->task))) > > /* > > - * 1. Kernel QPs should be visible in init namspace > only > > - * 2. Present only QPs visible in the current > namespace > > + * 1. Kern resources should be visible in init > > + * namspace only > > + * 2. Present only resources visible in the current > > + * namespace > > */ > > goto next; > > > > @@ -638,10 +650,10 @@ static int nldev_res_get_qp_dumpit(struct > sk_buff *skb, > > */ > > goto next; > > > > - qp = container_of(res, struct ib_qp, res); > > + filled = true; > > > > up_read(&device->res.rwsem); > > - ret = fill_res_qp_entry(skb, qp, port); > > + ret = fill_entry->fill_res_func(skb, cb, res, port); > > down_read(&device->res.rwsem); > > /* > > * Return resource back, but it won't be released till > > @@ -667,10 +679,10 @@ static int nldev_res_get_qp_dumpit(struct > sk_buff *skb, > > cb->args[0] = idx; > > > > /* > > - * No more QPs to fill, cancel the message and > > + * No more entries to fill, cancel the message and > > * return 0 to mark end of dumpit. > > */ > > - if (!qp) > > + if (!filled) > > goto err; > > > > put_device(&device->dev); > > @@ -688,6 +700,21 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > *skb, > > return ret; > > } > > > > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > > + [RDMA_RESTRACK_QP] = { > > + .fill_res_func = fill_res_qp_entry, > > + .res_type = RDMA_RESTRACK_QP, > > + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, > > + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, > > + }, > > +}; > > Steve, > > I imagined such function callback code a little bit simpler than it is now: > > nldev_res_get_XXX_dumpit { > initialization > checks > > ->callback(skb, RDMA_RESTRACK_XXX) > > close/error handling > } > > It will simplify the callback table a bit. That would work, at the expense of duplicating the parsing of the nl req and building of the nl reply table. I'll look into it. It might make things a little cleaner. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 27, 2018 at 09:32:49AM -0600, Steve Wise wrote: > > > > On Mon, Feb 26, 2018 at 03:22:14PM -0800, Steve Wise wrote: > > > Create a common dumpit function that can be used by all common > > resource > > > types. This reduces code replication and simplifies the code as we add > > > more resource types. > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > --- > > > drivers/infiniband/core/nldev.c | 61 +++++++++++++++++++++++++++++---- > > -------- > > > 1 file changed, 44 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > b/drivers/infiniband/core/nldev.c > > > index 5326a68..4f1cfa6 100644 > > > --- a/drivers/infiniband/core/nldev.c > > > +++ b/drivers/infiniband/core/nldev.c > > > @@ -212,10 +212,10 @@ static int fill_res_info(struct sk_buff *msg, > struct > > ib_device *device) > > > return ret; > > > } > > > > > > -static int fill_res_qp_entry(struct sk_buff *msg, > > > - struct ib_qp *qp, uint32_t port) > > > +static int fill_res_qp_entry(struct sk_buff *msg, struct > netlink_callback *cb, > > > + struct rdma_restrack_entry *res, uint32_t port) > > > { > > > - struct rdma_restrack_entry *res = &qp->res; > > > + struct ib_qp *qp = container_of(res, struct ib_qp, res); > > > struct ib_qp_init_attr qp_init_attr; > > > struct nlattr *entry_attr; > > > struct ib_qp_attr qp_attr; > > > @@ -558,8 +558,17 @@ static int nldev_res_get_dumpit(struct sk_buff > > *skb, > > > return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb); > > > } > > > > > > -static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > > > - struct netlink_callback *cb) > > > +struct nldev_fill_res_entry { > > > + int (*fill_res_func)(struct sk_buff *msg, struct netlink_callback > *cb, > > > + struct rdma_restrack_entry *res, u32 port); > > > + enum rdma_restrack_type res_type; > > > + enum rdma_nldev_attr nldev_attr; > > > + enum rdma_nldev_command nldev_cmd; > > > +}; > > > + > > > +static int res_get_common_dumpit(struct sk_buff *skb, > > > + struct netlink_callback *cb, > > > + struct nldev_fill_res_entry *fill_entry) > > > { > > > struct nlattr *tb[RDMA_NLDEV_ATTR_MAX]; > > > struct rdma_restrack_entry *res; > > > @@ -567,9 +576,9 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > > *skb, > > > struct nlattr *table_attr; > > > struct ib_device *device; > > > int start = cb->args[0]; > > > - struct ib_qp *qp = NULL; > > > struct nlmsghdr *nlh; > > > u32 index, port = 0; > > > + bool filled = false; > > > > > > err = nlmsg_parse(cb->nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, > > > nldev_policy, NULL); > > > @@ -601,7 +610,7 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > > *skb, > > > } > > > > > > nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh- > > >nlmsg_seq, > > > - RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, > > RDMA_NLDEV_CMD_RES_QP_GET), > > > + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, fill_entry- > > >nldev_cmd), > > > 0, NLM_F_MULTI); > > > > > > if (fill_nldev_handle(skb, device)) { > > > @@ -609,24 +618,27 @@ static int nldev_res_get_qp_dumpit(struct > > sk_buff *skb, > > > goto err; > > > } > > > > > > - table_attr = nla_nest_start(skb, RDMA_NLDEV_ATTR_RES_QP); > > > + table_attr = nla_nest_start(skb, fill_entry->nldev_attr); > > > if (!table_attr) { > > > ret = -EMSGSIZE; > > > goto err; > > > } > > > > > > down_read(&device->res.rwsem); > > > - hash_for_each_possible(device->res.hash, res, node, > > RDMA_RESTRACK_QP) { > > > + hash_for_each_possible(device->res.hash, res, node, > > > + fill_entry->res_type) { > > > if (idx < start) > > > goto next; > > > > > > if ((rdma_is_kernel_res(res) && > > > task_active_pid_ns(current) != &init_pid_ns) || > > > - (!rdma_is_kernel_res(res) && > > > - task_active_pid_ns(current) != task_active_pid_ns(res- > > >task))) > > > + (!rdma_is_kernel_res(res) && task_active_pid_ns(current) > > != > > > + task_active_pid_ns(res->task))) > > > /* > > > - * 1. Kernel QPs should be visible in init namspace > > only > > > - * 2. Present only QPs visible in the current > > namespace > > > + * 1. Kern resources should be visible in init > > > + * namspace only > > > + * 2. Present only resources visible in the current > > > + * namespace > > > */ > > > goto next; > > > > > > @@ -638,10 +650,10 @@ static int nldev_res_get_qp_dumpit(struct > > sk_buff *skb, > > > */ > > > goto next; > > > > > > - qp = container_of(res, struct ib_qp, res); > > > + filled = true; > > > > > > up_read(&device->res.rwsem); > > > - ret = fill_res_qp_entry(skb, qp, port); > > > + ret = fill_entry->fill_res_func(skb, cb, res, port); > > > down_read(&device->res.rwsem); > > > /* > > > * Return resource back, but it won't be released till > > > @@ -667,10 +679,10 @@ static int nldev_res_get_qp_dumpit(struct > > sk_buff *skb, > > > cb->args[0] = idx; > > > > > > /* > > > - * No more QPs to fill, cancel the message and > > > + * No more entries to fill, cancel the message and > > > * return 0 to mark end of dumpit. > > > */ > > > - if (!qp) > > > + if (!filled) > > > goto err; > > > > > > put_device(&device->dev); > > > @@ -688,6 +700,21 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > > *skb, > > > return ret; > > > } > > > > > > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > > > + [RDMA_RESTRACK_QP] = { > > > + .fill_res_func = fill_res_qp_entry, > > > + .res_type = RDMA_RESTRACK_QP, > > > + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, > > > + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, > > > + }, > > > +}; > > > > Steve, > > > > I imagined such function callback code a little bit simpler than it is > now: > > > > nldev_res_get_XXX_dumpit { > > initialization > > checks > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > close/error handling > > } > > > > It will simplify the callback table a bit. > > That would work, at the expense of duplicating the parsing of the nl req and > building of the nl reply table. I'll look into it. It might make things a > little cleaner. They can be static internal helpers, so no actual duplication will be. Thanks > >
On Tue, Feb 27, 2018 at 09:16:17PM +0200, Leon Romanovsky wrote: > On Tue, Feb 27, 2018 at 09:32:49AM -0600, Steve Wise wrote: > > > > > > On Mon, Feb 26, 2018 at 03:22:14PM -0800, Steve Wise wrote: > > > > Create a common dumpit function that can be used by all common > > > resource > > > > types. This reduces code replication and simplifies the code as we add > > > > more resource types. > > > > > > > > Signed-off-by: Steve Wise <swise@opengridcomputing.com> > > > > --- > > > > drivers/infiniband/core/nldev.c | 61 +++++++++++++++++++++++++++++---- > > > -------- > > > > 1 file changed, 44 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/core/nldev.c > > > b/drivers/infiniband/core/nldev.c > > > > index 5326a68..4f1cfa6 100644 > > > > --- a/drivers/infiniband/core/nldev.c > > > > +++ b/drivers/infiniband/core/nldev.c > > > > @@ -212,10 +212,10 @@ static int fill_res_info(struct sk_buff *msg, > > struct > > > ib_device *device) > > > > return ret; > > > > } > > > > > > > > -static int fill_res_qp_entry(struct sk_buff *msg, > > > > - struct ib_qp *qp, uint32_t port) > > > > +static int fill_res_qp_entry(struct sk_buff *msg, struct > > netlink_callback *cb, > > > > + struct rdma_restrack_entry *res, uint32_t port) > > > > { > > > > - struct rdma_restrack_entry *res = &qp->res; > > > > + struct ib_qp *qp = container_of(res, struct ib_qp, res); > > > > struct ib_qp_init_attr qp_init_attr; > > > > struct nlattr *entry_attr; > > > > struct ib_qp_attr qp_attr; > > > > @@ -558,8 +558,17 @@ static int nldev_res_get_dumpit(struct sk_buff > > > *skb, > > > > return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb); > > > > } > > > > > > > > -static int nldev_res_get_qp_dumpit(struct sk_buff *skb, > > > > - struct netlink_callback *cb) > > > > +struct nldev_fill_res_entry { > > > > + int (*fill_res_func)(struct sk_buff *msg, struct netlink_callback > > *cb, > > > > + struct rdma_restrack_entry *res, u32 port); > > > > + enum rdma_restrack_type res_type; > > > > + enum rdma_nldev_attr nldev_attr; > > > > + enum rdma_nldev_command nldev_cmd; > > > > +}; > > > > + > > > > +static int res_get_common_dumpit(struct sk_buff *skb, > > > > + struct netlink_callback *cb, > > > > + struct nldev_fill_res_entry *fill_entry) > > > > { > > > > struct nlattr *tb[RDMA_NLDEV_ATTR_MAX]; > > > > struct rdma_restrack_entry *res; > > > > @@ -567,9 +576,9 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > > > *skb, > > > > struct nlattr *table_attr; > > > > struct ib_device *device; > > > > int start = cb->args[0]; > > > > - struct ib_qp *qp = NULL; > > > > struct nlmsghdr *nlh; > > > > u32 index, port = 0; > > > > + bool filled = false; > > > > > > > > err = nlmsg_parse(cb->nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, > > > > nldev_policy, NULL); > > > > @@ -601,7 +610,7 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > > > *skb, > > > > } > > > > > > > > nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh- > > > >nlmsg_seq, > > > > - RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, > > > RDMA_NLDEV_CMD_RES_QP_GET), > > > > + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, fill_entry- > > > >nldev_cmd), > > > > 0, NLM_F_MULTI); > > > > > > > > if (fill_nldev_handle(skb, device)) { > > > > @@ -609,24 +618,27 @@ static int nldev_res_get_qp_dumpit(struct > > > sk_buff *skb, > > > > goto err; > > > > } > > > > > > > > - table_attr = nla_nest_start(skb, RDMA_NLDEV_ATTR_RES_QP); > > > > + table_attr = nla_nest_start(skb, fill_entry->nldev_attr); > > > > if (!table_attr) { > > > > ret = -EMSGSIZE; > > > > goto err; > > > > } > > > > > > > > down_read(&device->res.rwsem); > > > > - hash_for_each_possible(device->res.hash, res, node, > > > RDMA_RESTRACK_QP) { > > > > + hash_for_each_possible(device->res.hash, res, node, > > > > + fill_entry->res_type) { > > > > if (idx < start) > > > > goto next; > > > > > > > > if ((rdma_is_kernel_res(res) && > > > > task_active_pid_ns(current) != &init_pid_ns) || > > > > - (!rdma_is_kernel_res(res) && > > > > - task_active_pid_ns(current) != task_active_pid_ns(res- > > > >task))) > > > > + (!rdma_is_kernel_res(res) && task_active_pid_ns(current) > > > != > > > > + task_active_pid_ns(res->task))) > > > > /* > > > > - * 1. Kernel QPs should be visible in init namspace > > > only > > > > - * 2. Present only QPs visible in the current > > > namespace > > > > + * 1. Kern resources should be visible in init > > > > + * namspace only > > > > + * 2. Present only resources visible in the current > > > > + * namespace > > > > */ > > > > goto next; > > > > > > > > @@ -638,10 +650,10 @@ static int nldev_res_get_qp_dumpit(struct > > > sk_buff *skb, > > > > */ > > > > goto next; > > > > > > > > - qp = container_of(res, struct ib_qp, res); > > > > + filled = true; > > > > > > > > up_read(&device->res.rwsem); > > > > - ret = fill_res_qp_entry(skb, qp, port); > > > > + ret = fill_entry->fill_res_func(skb, cb, res, port); > > > > down_read(&device->res.rwsem); > > > > /* > > > > * Return resource back, but it won't be released till > > > > @@ -667,10 +679,10 @@ static int nldev_res_get_qp_dumpit(struct > > > sk_buff *skb, > > > > cb->args[0] = idx; > > > > > > > > /* > > > > - * No more QPs to fill, cancel the message and > > > > + * No more entries to fill, cancel the message and > > > > * return 0 to mark end of dumpit. > > > > */ > > > > - if (!qp) > > > > + if (!filled) > > > > goto err; > > > > > > > > put_device(&device->dev); > > > > @@ -688,6 +700,21 @@ static int nldev_res_get_qp_dumpit(struct sk_buff > > > *skb, > > > > return ret; > > > > } > > > > > > > > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > > > > + [RDMA_RESTRACK_QP] = { > > > > + .fill_res_func = fill_res_qp_entry, > > > > + .res_type = RDMA_RESTRACK_QP, > > > > + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, > > > > + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, > > > > + }, > > > > +}; > > > > > > Steve, > > > > > > I imagined such function callback code a little bit simpler than it is > > now: > > > > > > nldev_res_get_XXX_dumpit { > > > initialization > > > checks > > > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > > > close/error handling > > > } > > > > > > It will simplify the callback table a bit. > > > > That would work, at the expense of duplicating the parsing of the nl req and > > building of the nl reply table. I'll look into it. It might make things a > > little cleaner. > > They can be static internal helpers, so no actual duplication will be. And after more thinking, it seems that you don't need callback too, simple wrapper function will be enough (everything is declared in one file, no need to dynamically change function callback, export it to other modules e.t.c). Thanks > > Thanks > > > > >
> > > > Steve, > > > > > > > > I imagined such function callback code a little bit simpler than it is > > > now: > > > > > > > > nldev_res_get_XXX_dumpit { > > > > initialization > > > > checks > > > > > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > > > > > close/error handling > > > > } > > > > > > > > It will simplify the callback table a bit. > > > > > > That would work, at the expense of duplicating the parsing of the nl req and > > > building of the nl reply table. I'll look into it. It might make things a > > > little cleaner. > > > > They can be static internal helpers, so no actual duplication will be. > > And after more thinking, it seems that you don't need callback too, > simple wrapper function will be enough (everything is declared in one > file, no need to dynamically change function callback, export it to > other modules e.t.c). > So you propose, I think: remove the logic in res_get_common_dumpit() that requires passing it the nldev_cmd and nldev_attr values. Move these to helper functions and call them directly from the nldev_res_get_XXX_dumpit() functions. This allows removing the nldev_fill_res_entry struct entirely. But we still need to either pass the resource-specific fill function pointer to res_get_common_dumpit(), or have a helper function that switches on res_type and calls the appropriate fill function. Are you saying you propose the latter vs passing the function ptr? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > > Steve, > > > > > > > > > > I imagined such function callback code a little bit simpler than it is > > > > now: > > > > > > > > > > nldev_res_get_XXX_dumpit { > > > > > initialization > > > > > checks > > > > > > > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > > > > > > > close/error handling > > > > > } > > > > > > > > > > It will simplify the callback table a bit. > > > > > > > > That would work, at the expense of duplicating the parsing of the nl req > and > > > > building of the nl reply table. I'll look into it. It might make things a > > > > little cleaner. > > > > > > They can be static internal helpers, so no actual duplication will be. > > > > And after more thinking, it seems that you don't need callback too, > > simple wrapper function will be enough (everything is declared in one > > file, no need to dynamically change function callback, export it to > > other modules e.t.c). > > > > So you propose, I think: > > remove the logic in res_get_common_dumpit() that requires passing it the > nldev_cmd and nldev_attr values. Move these to helper functions and call them > directly from the nldev_res_get_XXX_dumpit() functions. This allows removing > the nldev_fill_res_entry struct entirely. > > But we still need to either pass the resource-specific fill function pointer to > res_get_common_dumpit(), or have a helper function that switches on res_type > and calls the appropriate fill function. Are you saying you propose the latter vs > passing the function ptr? > I'm inclined to leave the design as I have done it. I'm not sure having each nldev_res_get_XXX_dumpit() function do something like below makes it easier to read or more maintainable: parse_table() get_device_index() init_nlreply() fill_handle() start_nested_entry() res_get_common_dumpit(skb, cb, res_type) finalize_nlreply() -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 28, 2018 at 10:38:35AM -0600, Steve Wise wrote: > > > > > > > Steve, > > > > > > > > > > > > I imagined such function callback code a little bit simpler than > it is > > > > > now: > > > > > > > > > > > > nldev_res_get_XXX_dumpit { > > > > > > initialization > > > > > > checks > > > > > > > > > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > > > > > > > > > close/error handling > > > > > > } > > > > > > > > > > > > It will simplify the callback table a bit. > > > > > > > > > > That would work, at the expense of duplicating the parsing of the nl > req > > and > > > > > building of the nl reply table. I'll look into it. It might make > things a > > > > > little cleaner. > > > > > > > > They can be static internal helpers, so no actual duplication will be. > > > > > > And after more thinking, it seems that you don't need callback too, > > > simple wrapper function will be enough (everything is declared in one > > > file, no need to dynamically change function callback, export it to > > > other modules e.t.c). > > > > > > > So you propose, I think: > > > > remove the logic in res_get_common_dumpit() that requires passing it the > > nldev_cmd and nldev_attr values. Move these to helper functions and call > them > > directly from the nldev_res_get_XXX_dumpit() functions. This allows > removing > > the nldev_fill_res_entry struct entirely. > > > > But we still need to either pass the resource-specific fill function > pointer to > > res_get_common_dumpit(), or have a helper function that switches on > res_type > > and calls the appropriate fill function. Are you saying you propose the > latter vs > > passing the function ptr? > > > > I'm inclined to leave the design as I have done it. I'm not sure having > each nldev_res_get_XXX_dumpit() function do something like below makes it > easier to read or more maintainable: I like this version with the fill callback, makes a lot of sense to me. Having only fill vary per type seems good with the way things are now. But not sure what picture Leon has to compare it with.. BTW: +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { + [RDMA_RESTRACK_QP] = { Missing 'const' Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 28, 2018 at 10:33:46AM -0700, Jason Gunthorpe wrote: > On Wed, Feb 28, 2018 at 10:38:35AM -0600, Steve Wise wrote: > > > > > > > > > Steve, > > > > > > > > > > > > > > I imagined such function callback code a little bit simpler than > > it is > > > > > > now: > > > > > > > > > > > > > > nldev_res_get_XXX_dumpit { > > > > > > > initialization > > > > > > > checks > > > > > > > > > > > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > > > > > > > > > > > close/error handling > > > > > > > } > > > > > > > > > > > > > > It will simplify the callback table a bit. > > > > > > > > > > > > That would work, at the expense of duplicating the parsing of the nl > > req > > > and > > > > > > building of the nl reply table. I'll look into it. It might make > > things a > > > > > > little cleaner. > > > > > > > > > > They can be static internal helpers, so no actual duplication will be. > > > > > > > > And after more thinking, it seems that you don't need callback too, > > > > simple wrapper function will be enough (everything is declared in one > > > > file, no need to dynamically change function callback, export it to > > > > other modules e.t.c). > > > > > > > > > > So you propose, I think: > > > > > > remove the logic in res_get_common_dumpit() that requires passing it the > > > nldev_cmd and nldev_attr values. Move these to helper functions and call > > them > > > directly from the nldev_res_get_XXX_dumpit() functions. This allows > > removing > > > the nldev_fill_res_entry struct entirely. > > > > > > But we still need to either pass the resource-specific fill function > > pointer to > > > res_get_common_dumpit(), or have a helper function that switches on > > res_type > > > and calls the appropriate fill function. Are you saying you propose the > > latter vs > > > passing the function ptr? > > > > > > > I'm inclined to leave the design as I have done it. I'm not sure having > > each nldev_res_get_XXX_dumpit() function do something like below makes it > > easier to read or more maintainable: > > I like this version with the fill callback, makes a lot of sense to > me. Having only fill vary per type seems good with the way things are > now. > > But not sure what picture Leon has to compare it with.. > > BTW: > > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > + [RDMA_RESTRACK_QP] = { > > Missing 'const' I don't like this construction: +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { + [RDMA_RESTRACK_QP] = { + .fill_res_func = fill_res_qp_entry, + .res_type = RDMA_RESTRACK_QP, + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, + }, +}; Constant and duplicated fields: res_type == index == RDMA_RESTRACK_QP nldev_cmd, nldev_attr I had in mind very simple picture: static check_input(...) { ... } static fill_entries(..) { ... } static qp_dummpit_func(..) { check_input(QP, ...) fill_entries(QP, ...) return ..; } static pd_dummpit_func(..) { check_input(PD, ...) fill_entries(PD, ...) return ..; } e.t.c. Please don't over-complicate things where it is not necessary, all those objects with dump functions will be constant and we won't add many objects in near future. Thanks > > Jason > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Wed, Feb 28, 2018 at 10:33:46AM -0700, Jason Gunthorpe wrote: > > On Wed, Feb 28, 2018 at 10:38:35AM -0600, Steve Wise wrote: > > > > > > > > > > > Steve, > > > > > > > > > > > > > > > > I imagined such function callback code a little bit simpler than > > > it is > > > > > > > now: > > > > > > > > > > > > > > > > nldev_res_get_XXX_dumpit { > > > > > > > > initialization > > > > > > > > checks > > > > > > > > > > > > > > > > ->callback(skb, RDMA_RESTRACK_XXX) > > > > > > > > > > > > > > > > close/error handling > > > > > > > > } > > > > > > > > > > > > > > > > It will simplify the callback table a bit. > > > > > > > > > > > > > > That would work, at the expense of duplicating the parsing of the nl > > > req > > > > and > > > > > > > building of the nl reply table. I'll look into it. It might make > > > things a > > > > > > > little cleaner. > > > > > > > > > > > > They can be static internal helpers, so no actual duplication will be. > > > > > > > > > > And after more thinking, it seems that you don't need callback too, > > > > > simple wrapper function will be enough (everything is declared in one > > > > > file, no need to dynamically change function callback, export it to > > > > > other modules e.t.c). > > > > > > > > > > > > > So you propose, I think: > > > > > > > > remove the logic in res_get_common_dumpit() that requires passing it the > > > > nldev_cmd and nldev_attr values. Move these to helper functions and call > > > them > > > > directly from the nldev_res_get_XXX_dumpit() functions. This allows > > > removing > > > > the nldev_fill_res_entry struct entirely. > > > > > > > > But we still need to either pass the resource-specific fill function > > > pointer to > > > > res_get_common_dumpit(), or have a helper function that switches on > > > res_type > > > > and calls the appropriate fill function. Are you saying you propose the > > > latter vs > > > > passing the function ptr? > > > > > > > > > > I'm inclined to leave the design as I have done it. I'm not sure having > > > each nldev_res_get_XXX_dumpit() function do something like below makes it > > > easier to read or more maintainable: > > > > I like this version with the fill callback, makes a lot of sense to > > me. Having only fill vary per type seems good with the way things are > > now. > > > > But not sure what picture Leon has to compare it with.. > > > > BTW: > > > > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > > + [RDMA_RESTRACK_QP] = { > > > > Missing 'const' > > I don't like this construction: > +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { > + [RDMA_RESTRACK_QP] = { > + .fill_res_func = fill_res_qp_entry, > + .res_type = RDMA_RESTRACK_QP, > + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, > + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, > + }, > +}; > > Constant and duplicated fields: > res_type == index == RDMA_RESTRACK_QP > nldev_cmd, nldev_attr > I could remove res_type, and pass it to res_get_common_dumpit() vs passing the fill_res_entry struct ptr. > I had in mind very simple picture: > > static check_input(...) > { ... } > static fill_entries(..) > { ... } > > static qp_dummpit_func(..) > { > check_input(QP, ...) > fill_entries(QP, ...) > return ..; > } > static pd_dummpit_func(..) > { > check_input(PD, ...) > fill_entries(PD, ...) > return ..; > } > > e.t.c. > > Please don't over-complicate things where it is not necessary, all those > objects with dump functions will be constant and we won't add many objects > in near future. > Your proposed check_input() would have to take res_type, nldev_cmd, and nldev_attr, correct? I don't want to respin this to have you shoot it down again because I'm passing too many parameters... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 01, 2018 at 08:21:26AM +0200, Leon Romanovsky wrote: > static qp_dummpit_func(..) > { > check_input(QP, ...) > fill_entries(QP, ...) but that is not how it works, it is isn't fill_entires, it is singular fill_entry(). You'd have to do something crazy like static qp_dummpit_func(..) { check_input(QP, ...) for (xx = start_loop(..); !loop_end(xx); next_loop(xx)) fill_entry_body() Which seems much worse than what is already in the patch. The nice tidy 'fill_entry' function is perfect, all the boiler plate is in the common code. I agree the const struct could have fewer members though.. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On Thu, Mar 01, 2018 at 08:21:26AM +0200, Leon Romanovsky wrote: > > > static qp_dummpit_func(..) > > { > > check_input(QP, ...) > > fill_entries(QP, ...) > > but that is not how it works, it is isn't fill_entires, it is singular > fill_entry(). > > You'd have to do something crazy like > > static qp_dummpit_func(..) > { > check_input(QP, ...) > for (xx = start_loop(..); !loop_end(xx); next_loop(xx)) > fill_entry_body() > > Which seems much worse than what is already in the patch. The nice > tidy 'fill_entry' function is perfect, all the boiler plate is in the > common code. > > I agree the const struct could have fewer members though.. I've removed res_type. I can remove the other nlattr types and pass them as parameters. Is that better? res_get_common_dumpit(skb, cb, RDMA_RESTRACK_QP, RDMA_NLDEV_CMD_RES_QP_GET, RDMA_NLDEV_ATTR_RES_QP); Or just have helper functions that return the nldev cmd and attr given the res_type. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 5326a68..4f1cfa6 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -212,10 +212,10 @@ static int fill_res_info(struct sk_buff *msg, struct ib_device *device) return ret; } -static int fill_res_qp_entry(struct sk_buff *msg, - struct ib_qp *qp, uint32_t port) +static int fill_res_qp_entry(struct sk_buff *msg, struct netlink_callback *cb, + struct rdma_restrack_entry *res, uint32_t port) { - struct rdma_restrack_entry *res = &qp->res; + struct ib_qp *qp = container_of(res, struct ib_qp, res); struct ib_qp_init_attr qp_init_attr; struct nlattr *entry_attr; struct ib_qp_attr qp_attr; @@ -558,8 +558,17 @@ static int nldev_res_get_dumpit(struct sk_buff *skb, return ib_enum_all_devs(_nldev_res_get_dumpit, skb, cb); } -static int nldev_res_get_qp_dumpit(struct sk_buff *skb, - struct netlink_callback *cb) +struct nldev_fill_res_entry { + int (*fill_res_func)(struct sk_buff *msg, struct netlink_callback *cb, + struct rdma_restrack_entry *res, u32 port); + enum rdma_restrack_type res_type; + enum rdma_nldev_attr nldev_attr; + enum rdma_nldev_command nldev_cmd; +}; + +static int res_get_common_dumpit(struct sk_buff *skb, + struct netlink_callback *cb, + struct nldev_fill_res_entry *fill_entry) { struct nlattr *tb[RDMA_NLDEV_ATTR_MAX]; struct rdma_restrack_entry *res; @@ -567,9 +576,9 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, struct nlattr *table_attr; struct ib_device *device; int start = cb->args[0]; - struct ib_qp *qp = NULL; struct nlmsghdr *nlh; u32 index, port = 0; + bool filled = false; err = nlmsg_parse(cb->nlh, 0, tb, RDMA_NLDEV_ATTR_MAX - 1, nldev_policy, NULL); @@ -601,7 +610,7 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, } nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, - RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, RDMA_NLDEV_CMD_RES_QP_GET), + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV, fill_entry->nldev_cmd), 0, NLM_F_MULTI); if (fill_nldev_handle(skb, device)) { @@ -609,24 +618,27 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, goto err; } - table_attr = nla_nest_start(skb, RDMA_NLDEV_ATTR_RES_QP); + table_attr = nla_nest_start(skb, fill_entry->nldev_attr); if (!table_attr) { ret = -EMSGSIZE; goto err; } down_read(&device->res.rwsem); - hash_for_each_possible(device->res.hash, res, node, RDMA_RESTRACK_QP) { + hash_for_each_possible(device->res.hash, res, node, + fill_entry->res_type) { if (idx < start) goto next; if ((rdma_is_kernel_res(res) && task_active_pid_ns(current) != &init_pid_ns) || - (!rdma_is_kernel_res(res) && - task_active_pid_ns(current) != task_active_pid_ns(res->task))) + (!rdma_is_kernel_res(res) && task_active_pid_ns(current) != + task_active_pid_ns(res->task))) /* - * 1. Kernel QPs should be visible in init namspace only - * 2. Present only QPs visible in the current namespace + * 1. Kern resources should be visible in init + * namspace only + * 2. Present only resources visible in the current + * namespace */ goto next; @@ -638,10 +650,10 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, */ goto next; - qp = container_of(res, struct ib_qp, res); + filled = true; up_read(&device->res.rwsem); - ret = fill_res_qp_entry(skb, qp, port); + ret = fill_entry->fill_res_func(skb, cb, res, port); down_read(&device->res.rwsem); /* * Return resource back, but it won't be released till @@ -667,10 +679,10 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, cb->args[0] = idx; /* - * No more QPs to fill, cancel the message and + * No more entries to fill, cancel the message and * return 0 to mark end of dumpit. */ - if (!qp) + if (!filled) goto err; put_device(&device->dev); @@ -688,6 +700,21 @@ static int nldev_res_get_qp_dumpit(struct sk_buff *skb, return ret; } +static struct nldev_fill_res_entry fill_entries[RDMA_RESTRACK_MAX] = { + [RDMA_RESTRACK_QP] = { + .fill_res_func = fill_res_qp_entry, + .res_type = RDMA_RESTRACK_QP, + .nldev_cmd = RDMA_NLDEV_CMD_RES_QP_GET, + .nldev_attr = RDMA_NLDEV_ATTR_RES_QP, + }, +}; + +static int nldev_res_get_qp_dumpit(struct sk_buff *skb, + struct netlink_callback *cb) +{ + return res_get_common_dumpit(skb, cb, &fill_entries[RDMA_RESTRACK_QP]); +} + static const struct rdma_nl_cbs nldev_cb_table[RDMA_NLDEV_NUM_OPS] = { [RDMA_NLDEV_CMD_GET] = { .doit = nldev_get_doit,
Create a common dumpit function that can be used by all common resource types. This reduces code replication and simplifies the code as we add more resource types. Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/infiniband/core/nldev.c | 61 +++++++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 17 deletions(-)