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@xxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > 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
>
>
Attachment:
signature.asc
Description: PGP signature
