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