| Message ID | 20230720121829.566974-12-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: introduce dump selector attr and use it for per-instance dumps | expand |
On Thu, 20 Jul 2023 14:18:29 +0200 Jiri Pirko wrote: > Introduce a possibility for devlink object to expose attributes it > supports for selection of dumped objects. > > Use this by health reporter to indicate it supports port index based > selection of dump objects. Implement this selection mechanism in > devlink_nl_cmd_health_reporter_get_dump_one() This patch is not very clean. IMHO implementing the filters by skipping is not going to scale to reasonably complex filters. Isn't it better to add a .filter callback which will look at the about-to-be-dumped object and return true/false on whether it should be dumped?
Tue, Jul 25, 2023 at 08:48:03PM CEST, kuba@kernel.org wrote: >On Thu, 20 Jul 2023 14:18:29 +0200 Jiri Pirko wrote: >> Introduce a possibility for devlink object to expose attributes it >> supports for selection of dumped objects. >> >> Use this by health reporter to indicate it supports port index based >> selection of dump objects. Implement this selection mechanism in >> devlink_nl_cmd_health_reporter_get_dump_one() > >This patch is not very clean. IMHO implementing the filters by skipping >is not going to scale to reasonably complex filters. Isn't it better to I'm not sure what do you mean by skipping? There is not skipping. In case PORT_INDEX is passed in the selector, only that specific port is processed. No scale issues I see. Am I missing something? >add a .filter callback which will look at the about-to-be-dumped object >and return true/false on whether it should be dumped? No, that would not scale. Passing the selector attrs to the dump callback it better, as the dump callback according to the attrs can reach only what is needed, knowing the internals. But perhaps I don't understand correctly your suggestion.
On Mon, 31 Jul 2023 14:52:44 +0200 Jiri Pirko wrote: > >This patch is not very clean. IMHO implementing the filters by skipping > >is not going to scale to reasonably complex filters. Isn't it better to > > I'm not sure what do you mean by skipping? There is not skipping. In > case PORT_INDEX is passed in the selector, only that specific port is > processed. No scale issues I see. Am I missing something? > > > >add a .filter callback which will look at the about-to-be-dumped object > >and return true/false on whether it should be dumped? > > No, that would not scale. Passing the selector attrs to the dump > callback it better, as the dump callback according to the attrs can > reach only what is needed, knowing the internals. But perhaps I don't > understand correctly your suggestion. for_each_obj() { if (obj_dump_filtered(obj, dump_info)) // < run filter continue; // < skip object dump_one(obj) }
Mon, Jul 31, 2023 at 07:06:32PM CEST, kuba@kernel.org wrote: >On Mon, 31 Jul 2023 14:52:44 +0200 Jiri Pirko wrote: >> >This patch is not very clean. IMHO implementing the filters by skipping >> >is not going to scale to reasonably complex filters. Isn't it better to >> >> I'm not sure what do you mean by skipping? There is not skipping. In >> case PORT_INDEX is passed in the selector, only that specific port is >> processed. No scale issues I see. Am I missing something? >> >> >> >add a .filter callback which will look at the about-to-be-dumped object >> >and return true/false on whether it should be dumped? >> >> No, that would not scale. Passing the selector attrs to the dump >> callback it better, as the dump callback according to the attrs can >> reach only what is needed, knowing the internals. But perhaps I don't >> understand correctly your suggestion. > >for_each_obj() { > if (obj_dump_filtered(obj, dump_info)) // < run filter > continue; // < skip object > > dump_one(obj) I don't see how this would help. For example, passing PORT_INDEX, I know exactly what object to reach, according to this PORT_INDEX. Why to iterate over all of them and try the filter? Does not make sense to me. Maybe we are each understanding this feature differently. This is about passing keys which index the objects. It is always devlink handle, sometimes port index and I see another example in shared buffer index. That's about it. Basically user passes partial tuple of indexes. Example: devlink port show the key is: bus_name/dev_name/port_index user passes bus_name/dev_name, this is the selector, a partial key. The sophisticated filtering is not a focus of this patchset. User can do it putting bpf filter on the netlink socket.
On Tue, 1 Aug 2023 08:49:45 +0200 Jiri Pirko wrote: > >for_each_obj() { > > if (obj_dump_filtered(obj, dump_info)) // < run filter > > continue; // < skip object > > > > dump_one(obj) > > I don't see how this would help. For example, passing PORT_INDEX, I know > exactly what object to reach, according to this PORT_INDEX. Why to > iterate over all of them and try the filter? Does not make sense to me. > > Maybe we are each understanding this feature differently. This is about > passing keys which index the objects. It is always devlink handle, > sometimes port index and I see another example in shared buffer index. > That's about it. Basically user passes partial tuple of indexes. > Example: > devlink port show > the key is: bus_name/dev_name/port_index > user passes bus_name/dev_name, this is the selector, a partial key. > > The sophisticated filtering is not a focus of this patchset. User can do > it putting bpf filter on the netlink socket. Okay, I was trying to be helpful, I don't want to argue for a particular implementation. IMO what's posted is too ugly to be merged, please restructure it.
Tue, Aug 01, 2023 at 05:56:44PM CEST, kuba@kernel.org wrote: >On Tue, 1 Aug 2023 08:49:45 +0200 Jiri Pirko wrote: >> >for_each_obj() { >> > if (obj_dump_filtered(obj, dump_info)) // < run filter >> > continue; // < skip object >> > >> > dump_one(obj) >> >> I don't see how this would help. For example, passing PORT_INDEX, I know >> exactly what object to reach, according to this PORT_INDEX. Why to >> iterate over all of them and try the filter? Does not make sense to me. >> >> Maybe we are each understanding this feature differently. This is about >> passing keys which index the objects. It is always devlink handle, >> sometimes port index and I see another example in shared buffer index. >> That's about it. Basically user passes partial tuple of indexes. >> Example: >> devlink port show >> the key is: bus_name/dev_name/port_index >> user passes bus_name/dev_name, this is the selector, a partial key. >> >> The sophisticated filtering is not a focus of this patchset. User can do >> it putting bpf filter on the netlink socket. > >Okay, I was trying to be helpful, I don't want to argue for >a particular implementation. IMO what's posted is too ugly >to be merged, please restructure it. Ugly in which sense? What exactly needs to be restructured?
diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h index 168d36dbc6f7..510b66806341 100644 --- a/net/devlink/devl_internal.h +++ b/net/devlink/devl_internal.h @@ -115,6 +115,8 @@ struct devlink_nl_dump_state { struct devlink_cmd { int (*dump_one)(struct sk_buff *msg, struct devlink *devlink, struct netlink_callback *cb); + const u16 *dump_selector_attrs; + unsigned int dump_selector_attrs_count; }; extern const struct genl_small_ops devlink_nl_ops[40]; diff --git a/net/devlink/health.c b/net/devlink/health.c index 194340a8bb86..74d322ee5b83 100644 --- a/net/devlink/health.c +++ b/net/devlink/health.c @@ -390,12 +390,21 @@ devlink_nl_cmd_health_reporter_get_dump_one(struct sk_buff *msg, struct netlink_callback *cb) { struct devlink_nl_dump_state *state = devlink_dump_state(cb); + struct nlattr **selector = state->selector; struct devlink_health_reporter *reporter; + unsigned long port_index_end = ULONG_MAX; + unsigned long port_index_start = 0; struct devlink_port *port; unsigned long port_index; int idx = 0; int err; + if (selector && selector[DEVLINK_ATTR_PORT_INDEX]) { + port_index_start = nla_get_u32(selector[DEVLINK_ATTR_PORT_INDEX]); + port_index_end = port_index_start; + goto per_port_dump; + } + list_for_each_entry(reporter, &devlink->reporter_list, list) { if (idx < state->idx) { idx++; @@ -412,7 +421,9 @@ devlink_nl_cmd_health_reporter_get_dump_one(struct sk_buff *msg, } idx++; } - xa_for_each(&devlink->ports, port_index, port) { +per_port_dump: + xa_for_each_range(&devlink->ports, port_index, port, + port_index_start, port_index_end) { list_for_each_entry(reporter, &port->reporter_list, list) { if (idx < state->idx) { idx++; @@ -434,8 +445,14 @@ devlink_nl_cmd_health_reporter_get_dump_one(struct sk_buff *msg, return 0; } +static const u16 devl_cmd_health_reporter_dump_selector_attrs[] = { + DEVLINK_ATTR_PORT_INDEX, +}; + const struct devlink_cmd devl_cmd_health_reporter_get = { - .dump_one = devlink_nl_cmd_health_reporter_get_dump_one, + .dump_one = devlink_nl_cmd_health_reporter_get_dump_one, + .dump_selector_attrs = devl_cmd_health_reporter_dump_selector_attrs, + .dump_selector_attrs_count = ARRAY_SIZE(devl_cmd_health_reporter_dump_selector_attrs), }; int devlink_nl_cmd_health_reporter_set_doit(struct sk_buff *skb, diff --git a/net/devlink/netlink.c b/net/devlink/netlink.c index c2083398bd73..bd60d229cfbe 100644 --- a/net/devlink/netlink.c +++ b/net/devlink/netlink.c @@ -265,8 +265,16 @@ static void devlink_nl_policy_cpy(struct nla_policy *policy, unsigned int attr) static void devlink_nl_dump_selector_policy_init(const struct devlink_cmd *cmd, struct nla_policy *policy) { + int i; + devlink_nl_policy_cpy(policy, DEVLINK_ATTR_BUS_NAME); devlink_nl_policy_cpy(policy, DEVLINK_ATTR_DEV_NAME); + + for (i = 0; i < cmd->dump_selector_attrs_count; i++) { + unsigned int attr = cmd->dump_selector_attrs[i]; + + memcpy(&policy[attr], &devlink_nl_policy[attr], sizeof(*policy)); + } } static int devlink_nl_start(struct netlink_callback *cb)