All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v10 0/4] net sched actions: improve dump performance
@ 2017-06-11 11:53 Jamal Hadi Salim
  2017-06-11 11:53 ` [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS Jamal Hadi Salim
                   ` (4 more replies)
  0 siblings, 5 replies; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 11:53 UTC (permalink / raw)
  To: davem
  Cc: netdev, jiri, xiyou.wangcong, eric.dumazet, simon.horman, mrv,
	Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

Changes since v9:
-----------------

1) General consensus:
- remove again the use of BIT() to maintain uapi consistency ;->

1) Jiri:
- Add a new netlink type NLA_FLAG_BITS to check for valid bits 
  and use it instead of inline vetting.
  
Changes since v8:
-----------------

1) Jiri:
- Add back the use of BIT(). Eventually fix iproute2 instead
- Rename VALID_TCA_FLAGS to VALID_TCA_ROOT_FLAGS

Changes since v7:
-----------------

Jamal:
No changes.
Patch 1 went out twice. Resend without two copies of patch 1

changes since v6:
-----------------

1) DaveM:
New rules for netlink messages. From now on we are going to start
checking for bits that are not used and rejecting anything we dont
understand. In the future this is going to require major changes
to user space code (tc etc). This is just a start.

To quote, David:
"
 Again, bits you aren't using now, make sure userspace doesn't
   set them.  And if it does, reject.
"
Added checks for ensuring things work as above.

2) Jiri:
a)Fix the commit message to properly use "Fixes" description
b)Align assignments for nla_policy

Changes since v5:
----------------

0)
Remove use of BIT() because it is kernel specific. Requires a separate
patch (Jiri can submit that in his cleanups)

1)To paraphrase Eric D.

"memcpy(nla_data(count_attr), &cb->args[1], sizeof(u32));
wont work on 64bit BE machines because cb->args[1]
(which is 64 bit is larger in size than sizeof(u32))"

Fixed

2) Jiri Pirko

i) Spotted a bug fix mixed in the patch for wrong TLV
fix. Add patch 1/3 to address this. Make part of this
series because of dependencies.

ii) Rename ACT_LARGE_DUMP_ON -> TCA_FLAG_LARGE_DUMP_ON

iii) Satisfy Jiri's obsession against the noun "tcaa"
a)Rename struct nlattr *tcaa --> struct nlattr *tb
b)Rename TCAA_ACT_XXX -> TCA_ROOT_XXX

Changes since v4:
-----------------

1) Eric D.

pointed out that when all skb space is used up by the dump
there will be no space to insert the TCAA_ACT_COUNT attribute.

2) Jiri:

i) Change:

enum {
        TCAA_UNSPEC,
        TCAA_ACT_TAB,
        TCAA_ACT_FLAGS,
        TCAA_ACT_COUNT,
        TCAA_ACT_TIME_FILTER,
        __TCAA_MAX
};

#define TCAA_MAX (__TCAA_MAX - 1)
#define ACT_LARGE_DUMP_ON               (1 << 0)

to:
enum {
       TCAA_UNSPEC,
       TCAA_ACT_TAB,
#define TCA_ACT_TAB TCAA_ACT_TAB
       TCAA_ACT_FLAGS,
       TCAA_ACT_COUNT,
       __TCAA_MAX,
#define        TCAA_MAX (__TCAA_MAX - 1)
};

#define ACT_LARGE_DUMP_ON              BIT(0)

Jiri plans to followup with the rest of the code to make the
style consistent.

ii) Rename attribute TCAA_ACT_TIME_FILTER --> TCAA_ACT_TIME_DELTA

iii) Rename variable jiffy_filter --> jiffy_since
iv) Rename msecs_filter --> msecs_since
v) get rid of unused cb->args[0] and rename cb->args[4] to cb->args[0]

Earlier Changes
----------------
- Jiri mostly on names of things.

Jamal Hadi Salim (4):
  net netlink: Add new type NLA_FLAG_BITS
  net sched actions: Use proper root attribute table for actions
  net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  net sched actions: add time filter for action dumping

 include/net/netlink.h          | 11 +++++++
 include/uapi/linux/rtnetlink.h | 40 ++++++++++++++++++++++--
 lib/nlattr.c                   | 25 +++++++++++++++
 net/sched/act_api.c            | 70 +++++++++++++++++++++++++++++++++++-------
 4 files changed, 133 insertions(+), 13 deletions(-)

-- 
1.9.1

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-11 11:53 [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
@ 2017-06-11 11:53 ` Jamal Hadi Salim
  2017-06-11 13:49   ` Jiri Pirko
  2017-06-11 11:53 ` [PATCH net-next v10 2/4] net sched actions: Use proper root attribute table for actions Jamal Hadi Salim
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 11:53 UTC (permalink / raw)
  To: davem
  Cc: netdev, jiri, xiyou.wangcong, eric.dumazet, simon.horman, mrv,
	Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

Generic bitflags attribute content sent to the kernel by user.
With this type the user can either set or unset a flag in the
kernel.

The nla_flag_values is a bitmap that defines the values being set
The nla_flag_selector is a bitmask that defines which value is legit.

A check is made to ensure the rules that a kernel subsystem always
conforms to bitflags the kernel already knows about. i.e
if the user tries to set a bit flag that is not understood then
the _it will be rejected_.

In the most basic form, the user specifies the attribute policy as:
[ATTR_GOO] = { .type = NLA_FLAG_BITS, .validation_data = &myvalidflags },

where myvalidflags is the bit mask of the flags the kernel understands.

If the user _does not_ provide myvalidflags then the attribute will
also be rejected.

Examples:
nla_flag_values = 0x0, and nla_flag_selector = 0x1
implies we are selecting bit 1 and we want to set its value to 0.

nla_flag_values = 0x2, and nla_flag_selector = 0x2
implies we are selecting bit 2 and we want to set its value to 1.

This patch also provides an extra feature: a validation callback
that could be speaciliazed for other types.
This feature is intended to be used by a kernel subsystem to check
for a combination of bits being present. Example "bit x is valid
only if bit y and z are present".

So a kernel subsystem could specify validation rules of the following
nature:

[ATTR_GOO] = { .type = MYTYPE,
	       .validation_data = &myvalidation_data,
               .validate_content = mycontent_validator },

With validator callback looking like:

int mycontent_validator(const struct nlattr *nla, void *valid_data)
{
       const struct myattribute *user_data = nla_data(nla);
       struct myvalidation_struct *valid_data_constraint = valid_data;

      ... return appropriate error code etc ...
}


Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/net/netlink.h          | 11 +++++++++++
 include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
 lib/nlattr.c                   | 25 +++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0170917..8ab9784 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -6,6 +6,11 @@
 #include <linux/jiffies.h>
 #include <linux/in6.h>
 
+struct nla_bit_flags {
+	u32 nla_flag_values;
+	u32 nla_flag_selector;
+};
+
 /* ========================================================================
  *         Netlink Messages and Attributes Interface (As Seen On TV)
  * ------------------------------------------------------------------------
@@ -178,6 +183,7 @@ enum {
 	NLA_S16,
 	NLA_S32,
 	NLA_S64,
+	NLA_FLAG_BITS,
 	__NLA_TYPE_MAX,
 };
 
@@ -206,6 +212,7 @@ enum {
  *    NLA_MSECS            Leaving the length field zero will verify the
  *                         given type fits, using it verifies minimum length
  *                         just like "All other"
+ *    NLA_FLAG_BITS        A bitmap/bitselector attribute
  *    All other            Minimum length of attribute payload
  *
  * Example:
@@ -213,11 +220,15 @@ enum {
  * 	[ATTR_FOO] = { .type = NLA_U16 },
  *	[ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
  *	[ATTR_BAZ] = { .len = sizeof(struct mystruct) },
+ *	[ATTR_GOO] = { .type = NLA_FLAG_BITS, .validation_data = &myvalidflags },
  * };
  */
 struct nla_policy {
 	u16		type;
 	u16		len;
+	void            *validation_data;
+	int             (*validate_content)(const struct nlattr *nla,
+					    const void *validation_data);
 };
 
 /**
diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 564790e..8f07957 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -179,6 +179,23 @@ struct rtattr {
 #define RTA_DATA(rta)   ((void*)(((char*)(rta)) + RTA_LENGTH(0)))
 #define RTA_PAYLOAD(rta) ((int)((rta)->rta_len) - RTA_LENGTH(0))
 
+/* Generic bitflags attribute content sent to the kernel.
+ *
+ * The nla_flag_values is a bitmap that defines the values being set
+ * The nla_flag_selector is a bitmask that defines which value is legit
+ *
+ * Examples:
+ *  nla_flag_values = 0x0, and nla_flag_selector = 0x1
+ *  implies we are selecting bit 1 and we want to set its value to 0.
+ *
+ *  nla_flag_values = 0x2, and nla_flag_selector = 0x2
+ *  implies we are selecting bit 2 and we want to set its value to 1.
+ *
+ */
+struct __nla_bit_flags {
+	__u32 nla_flag_values;
+	__u32 nla_flag_selector;
+};
 
 
 
diff --git a/lib/nlattr.c b/lib/nlattr.c
index a7e0b16..78fed43 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -27,6 +27,21 @@
 	[NLA_S64]	= sizeof(s64),
 };
 
+static int validate_nla_bit_flags(const struct nlattr *nla, void *valid_data)
+{
+	const struct nla_bit_flags *nbf = nla_data(nla);
+	u32 *valid_flags_mask = valid_data;
+
+	if (!valid_data)
+		return -EINVAL;
+
+
+	if (nbf->nla_flag_values & ~*valid_flags_mask)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int validate_nla(const struct nlattr *nla, int maxtype,
 			const struct nla_policy *policy)
 {
@@ -46,6 +61,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			return -ERANGE;
 		break;
 
+	case NLA_FLAG_BITS:
+		if (attrlen != 8) /* 2 x 32 bits */
+			return -ERANGE;
+
+		return validate_nla_bit_flags(nla, pt->validation_data);
+		break;
+
 	case NLA_NUL_STRING:
 		if (pt->len)
 			minlen = min_t(int, attrlen, pt->len + 1);
@@ -103,6 +125,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
 			return -ERANGE;
 	}
 
+	if (pt->validate_content)
+		return pt->validate_content(nla, pt->validation_data);
+
 	return 0;
 }
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH net-next v10 2/4] net sched actions: Use proper root attribute table for actions
  2017-06-11 11:53 [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
  2017-06-11 11:53 ` [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS Jamal Hadi Salim
@ 2017-06-11 11:53 ` Jamal Hadi Salim
  2017-06-11 13:55   ` Jiri Pirko
  2017-06-11 11:53 ` [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch Jamal Hadi Salim
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 11:53 UTC (permalink / raw)
  To: davem
  Cc: netdev, jiri, xiyou.wangcong, eric.dumazet, simon.horman, mrv,
	Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

Bug fix for an issue which has been around for about a decade.
We got away with it because the enumeration was larger than needed.

Fixes: 7ba699c604ab ("[NET_SCHED]: Convert actions from rtnetlink to new netlink API")
Suggested-by: Jiri Pirko <jiri@mellanox.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/act_api.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index aed6cf2..400eb6e 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1072,7 +1072,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 			 struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
-	struct nlattr *tca[TCA_ACT_MAX + 1];
+	struct nlattr *tca[TCAA_MAX + 1];
 	u32 portid = skb ? NETLINK_CB(skb).portid : 0;
 	int ret = 0, ovr = 0;
 
@@ -1080,7 +1080,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ACT_MAX, NULL,
+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
 			  extack);
 	if (ret < 0)
 		return ret;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-11 11:53 [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
  2017-06-11 11:53 ` [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS Jamal Hadi Salim
  2017-06-11 11:53 ` [PATCH net-next v10 2/4] net sched actions: Use proper root attribute table for actions Jamal Hadi Salim
@ 2017-06-11 11:53 ` Jamal Hadi Salim
  2017-06-11 14:13   ` Jiri Pirko
  2017-06-11 11:53 ` [PATCH net-next v10 4/4] net sched actions: add time filter for action dumping Jamal Hadi Salim
  2017-06-11 12:06 ` [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
  4 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 11:53 UTC (permalink / raw)
  To: davem
  Cc: netdev, jiri, xiyou.wangcong, eric.dumazet, simon.horman, mrv,
	Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

When you dump hundreds of thousands of actions, getting only 32 per
dump batch even when the socket buffer and memory allocations allow
is inefficient.

With this change, the user will get as many as possibly fitting
within the given constraints available to the kernel.

The top level action TLV space is extended. An attribute
TCA_ROOT_FLAGS is used to carry flags; flag TCA_FLAG_LARGE_DUMP_ON
is set by the user indicating the user is capable of processing
these large dumps. Older user space which doesnt set this flag
doesnt get the large (than 32) batches.
The kernel uses the TCA_ROOT_COUNT attribute to tell the user how many
actions are put in a single batch. As such user space app knows how long
to iterate (independent of the type of action being dumped)
instead of hardcoded maximum of 32 thus maintaining backward compat.

Some results dumping 1.5M actions below:
first an unpatched tc which doesnt understand these features...

prompt$ time -p tc actions ls action gact | grep index | wc -l
1500000
real 1388.43
user 2.07
sys 1386.79

Now lets see a patched tc which sets the correct flags when requesting
a dump:

prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
1500000
real 178.13
user 2.02
sys 176.96

That is about 8x performance improvement for tc app which sets its
receive buffer to about 32K.

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/uapi/linux/rtnetlink.h | 22 +++++++++++++++++--
 net/sched/act_api.c            | 50 +++++++++++++++++++++++++++++++++---------
 2 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 8f07957..7d2030f 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -693,10 +693,28 @@ struct tcamsg {
 	unsigned char	tca__pad1;
 	unsigned short	tca__pad2;
 };
+
+enum {
+	TCA_ROOT_UNSPEC,
+	TCA_ROOT_TAB,
+#define TCA_ACT_TAB TCA_ROOT_TAB
+#define TCAA_MAX TCA_ROOT_TAB
+	TCA_ROOT_FLAGS,
+	TCA_ROOT_COUNT,
+	__TCA_ROOT_MAX,
+#define	TCA_ROOT_MAX (__TCA_ROOT_MAX - 1)
+};
+
 #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
 #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
-#define TCAA_MAX 1
+/* tcamsg flags stored in attribute TCA_ROOT_FLAGS
+ *
+ * TCA_FLAG_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
+ * actions in a dump. All dump responses will contain the number of actions
+ * being dumped stored in for user app's consumption in TCA_ROOT_COUNT
+ *
+ */
+#define TCA_FLAG_LARGE_DUMP_ON		(1 << 0)
 
 /* New extended info filters for IFLA_EXT_MASK */
 #define RTEXT_FILTER_VF		(1 << 0)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 400eb6e..935dc19 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -110,6 +110,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			   struct netlink_callback *cb)
 {
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
+	u32 act_flags = cb->args[2];
 	struct nlattr *nest;
 
 	spin_lock_bh(&hinfo->lock);
@@ -138,14 +139,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			}
 			nla_nest_end(skb, nest);
 			n_i++;
-			if (n_i >= TCA_ACT_MAX_PRIO)
+			if (!(act_flags & TCA_FLAG_LARGE_DUMP_ON) &&
+			    n_i >= TCA_ACT_MAX_PRIO)
 				goto done;
 		}
 	}
 done:
 	spin_unlock_bh(&hinfo->lock);
-	if (n_i)
+	if (n_i) {
 		cb->args[0] += n_i;
+		if (act_flags & TCA_FLAG_LARGE_DUMP_ON)
+			cb->args[1] = n_i;
+	}
 	return n_i;
 
 nla_put_failure:
@@ -1068,11 +1073,17 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 	return tcf_add_notify(net, n, &actions, portid);
 }
 
+static u32 allowed_flags = TCA_FLAG_LARGE_DUMP_ON;
+static const struct nla_policy tcaa_policy[TCA_ROOT_MAX + 1] = {
+	[TCA_ROOT_FLAGS] = { .type = NLA_FLAG_BITS,
+			     .validation_data = &allowed_flags },
+};
+
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 			 struct netlink_ext_ack *extack)
 {
 	struct net *net = sock_net(skb->sk);
-	struct nlattr *tca[TCAA_MAX + 1];
+	struct nlattr *tca[TCA_ROOT_MAX + 1];
 	u32 portid = skb ? NETLINK_CB(skb).portid : 0;
 	int ret = 0, ovr = 0;
 
@@ -1080,7 +1091,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	    !netlink_capable(skb, CAP_NET_ADMIN))
 		return -EPERM;
 
-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ROOT_MAX, NULL,
 			  extack);
 	if (ret < 0)
 		return ret;
@@ -1121,16 +1132,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
 	return ret;
 }
 
-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
+static struct nlattr *find_dump_kind(struct nlattr **nla)
 {
 	struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
-	struct nlattr *nla[TCAA_MAX + 1];
 	struct nlattr *kind;
 
-	if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
-			NULL, NULL) < 0)
-		return NULL;
 	tb1 = nla[TCA_ACT_TAB];
 	if (tb1 == NULL)
 		return NULL;
@@ -1157,8 +1164,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	struct tc_action_ops *a_o;
 	int ret = 0;
 	struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
-	struct nlattr *kind = find_dump_kind(cb->nlh);
+	struct __nla_bit_flags select_flags;
+	struct nlattr *count_attr = NULL;
+	struct nlattr *tb[TCA_ROOT_MAX + 1];
+	struct nlattr *kind = NULL;
+	u32 act_count = 0;
+
+	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tb, TCA_ROOT_MAX,
+			  tcaa_policy, NULL);
+	if (ret < 0)
+		return ret;
 
+	kind = find_dump_kind(tb);
 	if (kind == NULL) {
 		pr_info("tc_dump_action: action bad kind\n");
 		return 0;
@@ -1168,14 +1185,24 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (a_o == NULL)
 		return 0;
 
+	if (tb[TCA_ROOT_FLAGS])
+		nla_memcpy(&select_flags, tb[TCA_ROOT_FLAGS],
+			   sizeof(select_flags));
+
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
 		goto out_module_put;
+
+	cb->args[2] = select_flags.nla_flag_values;
 	t = nlmsg_data(nlh);
 	t->tca_family = AF_UNSPEC;
 	t->tca__pad1 = 0;
 	t->tca__pad2 = 0;
+	count_attr = nla_reserve(skb, TCA_ROOT_COUNT, sizeof(u32));
+	if (!count_attr)
+		goto out_module_put;
+
 
 	nest = nla_nest_start(skb, TCA_ACT_TAB);
 	if (nest == NULL)
@@ -1188,6 +1215,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	if (ret > 0) {
 		nla_nest_end(skb, nest);
 		ret = skb->len;
+		act_count = cb->args[1];
+		memcpy(nla_data(count_attr), &act_count, sizeof(u32));
+		cb->args[1] = 0;
 	} else
 		nlmsg_trim(skb, b);
 
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH net-next v10 4/4] net sched actions: add time filter for action dumping
  2017-06-11 11:53 [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
                   ` (2 preceding siblings ...)
  2017-06-11 11:53 ` [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch Jamal Hadi Salim
@ 2017-06-11 11:53 ` Jamal Hadi Salim
  2017-06-11 12:06 ` [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
  4 siblings, 0 replies; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 11:53 UTC (permalink / raw)
  To: davem
  Cc: netdev, jiri, xiyou.wangcong, eric.dumazet, simon.horman, mrv,
	Jamal Hadi Salim

From: Jamal Hadi Salim <jhs@mojatatu.com>

This adds support for filtering based on time since last used.
When we are dumping a large number of actions it is useful to
have the option of filtering based on when the action was last
used to reduce the amount of data crossing to user space.

With this patch the user space app sets the TCA_ROOT_TIME_DELTA
attribute with the value in milliseconds with "time of interest
since now".  The kernel converts this to jiffies and does the
filtering comparison matching entries that have seen activity
since then and returns them to user space.
Old kernels and old tc continue to work in legacy mode since
they dont specify this attribute.

Some example (we have 400 actions bound to 400 filters); at
installation time using  hacked tc which sets the time of
interest to 120 seconds:
prompt$ hackedtc actions ls action gact | grep index | wc -l
400

go get some coffee and wait for > 120 seconds and try again:

prompt$ hackedtc actions ls action gact | grep index | wc -l
0

Lets see a filter bound to one of these actions:
..
filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10  (rule hit 2 success 1)
  match 7f000002/ffffffff at 12 (success 1 )
    action order 1: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1145 sec used 802 sec
    Action statistics:
    Sent 84 bytes 1 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0
....

that coffee took long, no? It was good.

Now lets ping -c 1 127.0.0.2, then run the actions again:
prompt$ hackedtc actions ls action gact | grep index | wc -l
1

More details please:

prompt$ hackedtc -s actions ls action gact

    action order 0: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1270 sec used 30 sec
    Action statistics:
    Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0

And the filter?

filter pref 10 u32
filter pref 10 u32 fh 800: ht divisor 1
filter pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:10  (rule hit 4 success 2)
  match 7f000002/ffffffff at 12 (success 2 )
    action order 1: gact action pass
     random type none pass val 0
     index 23 ref 2 bind 1 installed 1324 sec used 84 sec
    Action statistics:
    Sent 168 bytes 2 pkt (dropped 0, overlimits 0 requeues 0)
    backlog 0b 0p requeues 0

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 include/uapi/linux/rtnetlink.h |  1 +
 net/sched/act_api.c            | 20 +++++++++++++++++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 7d2030f..988ebc2 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -701,6 +701,7 @@ enum {
 #define TCAA_MAX TCA_ROOT_TAB
 	TCA_ROOT_FLAGS,
 	TCA_ROOT_COUNT,
+	TCA_ROOT_TIME_DELTA, /* in msecs */
 	__TCA_ROOT_MAX,
 #define	TCA_ROOT_MAX (__TCA_ROOT_MAX - 1)
 };
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 935dc19..aa9549f 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -111,6 +111,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 {
 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
 	u32 act_flags = cb->args[2];
+	unsigned long jiffy_since = cb->args[3];
 	struct nlattr *nest;
 
 	spin_lock_bh(&hinfo->lock);
@@ -128,6 +129,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 			if (index < s_i)
 				continue;
 
+			if (jiffy_since &&
+			    time_after(jiffy_since,
+				       (unsigned long)p->tcfa_tm.lastuse))
+				continue;
+
 			nest = nla_nest_start(skb, n_i);
 			if (nest == NULL)
 				goto nla_put_failure;
@@ -145,9 +151,11 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
 		}
 	}
 done:
+	if (index >= 0)
+		cb->args[0] = index + 1;
+
 	spin_unlock_bh(&hinfo->lock);
 	if (n_i) {
-		cb->args[0] += n_i;
 		if (act_flags & TCA_FLAG_LARGE_DUMP_ON)
 			cb->args[1] = n_i;
 	}
@@ -1077,6 +1085,7 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
 static const struct nla_policy tcaa_policy[TCA_ROOT_MAX + 1] = {
 	[TCA_ROOT_FLAGS] = { .type = NLA_FLAG_BITS,
 			     .validation_data = &allowed_flags },
+	[TCA_ROOT_TIME_DELTA]      = { .type = NLA_U32 },
 };
 
 static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
@@ -1167,7 +1176,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 	struct __nla_bit_flags select_flags;
 	struct nlattr *count_attr = NULL;
 	struct nlattr *tb[TCA_ROOT_MAX + 1];
+	unsigned long jiffy_since = 0;
 	struct nlattr *kind = NULL;
+	u32 msecs_since = 0;
 	u32 act_count = 0;
 
 	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tb, TCA_ROOT_MAX,
@@ -1189,12 +1200,19 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
 		nla_memcpy(&select_flags, tb[TCA_ROOT_FLAGS],
 			   sizeof(select_flags));
 
+	if (tb[TCA_ROOT_TIME_DELTA])
+		msecs_since = nla_get_u32(tb[TCA_ROOT_TIME_DELTA]);
+
 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			cb->nlh->nlmsg_type, sizeof(*t), 0);
 	if (!nlh)
 		goto out_module_put;
 
+	if (msecs_since)
+		jiffy_since = jiffies - msecs_to_jiffies(msecs_since);
+
 	cb->args[2] = select_flags.nla_flag_values;
+	cb->args[3] = jiffy_since;
 	t = nlmsg_data(nlh);
 	t->tca_family = AF_UNSPEC;
 	t->tca__pad1 = 0;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 0/4] net sched actions: improve dump performance
  2017-06-11 11:53 [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
                   ` (3 preceding siblings ...)
  2017-06-11 11:53 ` [PATCH net-next v10 4/4] net sched actions: add time filter for action dumping Jamal Hadi Salim
@ 2017-06-11 12:06 ` Jamal Hadi Salim
  4 siblings, 0 replies; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 12:06 UTC (permalink / raw)
  To: davem; +Cc: netdev, jiri, xiyou.wangcong, eric.dumazet, simon.horman, mrv

[-- Attachment #1: Type: text/plain, Size: 68 bytes --]


Attached iproute2 used for testing these features..

cheers,
jamal

[-- Attachment #2: patch-large-dump-iproute2 --]
[-- Type: text/plain, Size: 13447 bytes --]

diff --git a/Makefile b/Makefile
index 18de7dc..ed3c10a 100644
--- a/Makefile
+++ b/Makefile
@@ -45,7 +45,7 @@ HOSTCC ?= $(CC)
 DEFINES += -D_GNU_SOURCE
 # Turn on transparent support for LFS
 DEFINES += -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE
-CCOPTS = -O2
+CCOPTS = -g
 WFLAGS := -Wall -Wstrict-prototypes  -Wmissing-prototypes
 WFLAGS += -Wmissing-declarations -Wold-style-definition -Wformat=2
 
diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index a96db83..988ebc2 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -1,5 +1,5 @@
-#ifndef __LINUX_RTNETLINK_H
-#define __LINUX_RTNETLINK_H
+#ifndef _UAPI__LINUX_RTNETLINK_H
+#define _UAPI__LINUX_RTNETLINK_H
 
 #include <linux/types.h>
 #include <linux/netlink.h>
@@ -179,6 +179,23 @@ struct rtattr {
 #define RTA_DATA(rta)   ((void*)(((char*)(rta)) + RTA_LENGTH(0)))
 #define RTA_PAYLOAD(rta) ((int)((rta)->rta_len) - RTA_LENGTH(0))
 
+/* Generic bitflags attribute content sent to the kernel.
+ *
+ * The nla_flag_values is a bitmap that defines the values being set
+ * The nla_flag_selector is a bitmask that defines which value is legit
+ *
+ * Examples:
+ *  nla_flag_values = 0x0, and nla_flag_selector = 0x1
+ *  implies we are selecting bit 1 and we want to set its value to 0.
+ *
+ *  nla_flag_values = 0x2, and nla_flag_selector = 0x2
+ *  implies we are selecting bit 2 and we want to set its value to 1.
+ *
+ */
+struct __nla_bit_flags {
+	__u32 nla_flag_values;
+	__u32 nla_flag_selector;
+};
 
 
 
@@ -278,6 +295,7 @@ enum rt_scope_t {
 #define RTM_F_EQUALIZE		0x400	/* Multipath equalizer: NI	*/
 #define RTM_F_PREFIX		0x800	/* Prefix addresses		*/
 #define RTM_F_LOOKUP_TABLE	0x1000	/* set rtm_table to FIB lookup result */
+#define RTM_F_FIB_MATCH	        0x2000	/* return full fib lookup match */
 
 /* Reserved table identifiers */
 
@@ -549,6 +567,7 @@ enum {
 	TCA_STAB,
 	TCA_PAD,
 	TCA_DUMP_INVISIBLE,
+	TCA_CHAIN,
 	__TCA_MAX
 };
 
@@ -581,6 +600,7 @@ enum {
 
 #define NDUSEROPT_MAX	(__NDUSEROPT_MAX - 1)
 
+#ifndef __KERNEL__
 /* RTnetlink multicast groups - backwards compatibility for userspace */
 #define RTMGRP_LINK		1
 #define RTMGRP_NOTIFY		2
@@ -601,6 +621,7 @@ enum {
 #define RTMGRP_DECnet_ROUTE     0x4000
 
 #define RTMGRP_IPV6_PREFIX	0x20000
+#endif
 
 /* RTnetlink multicast groups */
 enum rtnetlink_groups {
@@ -672,10 +693,29 @@ struct tcamsg {
 	unsigned char	tca__pad1;
 	unsigned short	tca__pad2;
 };
+
+enum {
+	TCA_ROOT_UNSPEC,
+	TCA_ROOT_TAB,
+#define TCA_ACT_TAB TCA_ROOT_TAB
+#define TCAA_MAX TCA_ROOT_TAB
+	TCA_ROOT_FLAGS,
+	TCA_ROOT_COUNT,
+	TCA_ROOT_TIME_DELTA, /* in msecs */
+	__TCA_ROOT_MAX,
+#define	TCA_ROOT_MAX (__TCA_ROOT_MAX - 1)
+};
+
 #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
 #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
-#define TCAA_MAX 1
+/* tcamsg flags stored in attribute TCA_ROOT_FLAGS
+ *
+ * TCA_FLAG_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
+ * actions in a dump. All dump responses will contain the number of actions
+ * being dumped stored in for user app's consumption in TCA_ROOT_COUNT
+ *
+ */
+#define TCA_FLAG_LARGE_DUMP_ON		(1 << 0)
 
 /* New extended info filters for IFLA_EXT_MASK */
 #define RTEXT_FILTER_VF		(1 << 0)
@@ -687,4 +727,4 @@ struct tcamsg {
 
 
 
-#endif /* __LINUX_RTNETLINK_H */
+#endif /* _UAPI__LINUX_RTNETLINK_H */
diff --git a/tc/f_basic.c b/tc/f_basic.c
index d663668..8370ea6 100644
--- a/tc/f_basic.c
+++ b/tc/f_basic.c
@@ -135,7 +135,7 @@ static int basic_print_opt(struct filter_util *qu, FILE *f,
 	}
 
 	if (tb[TCA_BASIC_ACT]) {
-		tc_print_action(f, tb[TCA_BASIC_ACT]);
+		tc_print_action(f, tb[TCA_BASIC_ACT], 0);
 	}
 
 	return 0;
diff --git a/tc/f_bpf.c b/tc/f_bpf.c
index 75c44c0..6581293 100644
--- a/tc/f_bpf.c
+++ b/tc/f_bpf.c
@@ -236,7 +236,7 @@ static int bpf_print_opt(struct filter_util *qu, FILE *f,
 	}
 
 	if (tb[TCA_BPF_ACT])
-		tc_print_action(f, tb[TCA_BPF_ACT]);
+		tc_print_action(f, tb[TCA_BPF_ACT], 0);
 
 	return 0;
 }
diff --git a/tc/f_cgroup.c b/tc/f_cgroup.c
index ecf9909..633700e 100644
--- a/tc/f_cgroup.c
+++ b/tc/f_cgroup.c
@@ -102,7 +102,7 @@ static int cgroup_print_opt(struct filter_util *qu, FILE *f,
 	}
 
 	if (tb[TCA_CGROUP_ACT])
-		tc_print_action(f, tb[TCA_CGROUP_ACT]);
+		tc_print_action(f, tb[TCA_CGROUP_ACT], 0);
 
 	return 0;
 }
diff --git a/tc/f_flow.c b/tc/f_flow.c
index 09ddcaa..b157104 100644
--- a/tc/f_flow.c
+++ b/tc/f_flow.c
@@ -347,7 +347,7 @@ static int flow_print_opt(struct filter_util *fu, FILE *f, struct rtattr *opt,
 		tc_print_police(f, tb[TCA_FLOW_POLICE]);
 	if (tb[TCA_FLOW_ACT]) {
 		fprintf(f, "\n");
-		tc_print_action(f, tb[TCA_FLOW_ACT]);
+		tc_print_action(f, tb[TCA_FLOW_ACT], 0);
 	}
 	return 0;
 }
diff --git a/tc/f_flower.c b/tc/f_flower.c
index ebc63ca..d015ade 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -1179,7 +1179,7 @@ static int flower_print_opt(struct filter_util *qu, FILE *f,
 	}
 
 	if (tb[TCA_FLOWER_ACT])
-		tc_print_action(f, tb[TCA_FLOWER_ACT]);
+		tc_print_action(f, tb[TCA_FLOWER_ACT], 0);
 
 	return 0;
 }
diff --git a/tc/f_fw.c b/tc/f_fw.c
index 790bef9..c39789b 100644
--- a/tc/f_fw.c
+++ b/tc/f_fw.c
@@ -160,7 +160,7 @@ static int fw_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt, __u
 
 	if (tb[TCA_FW_ACT]) {
 		fprintf(f, "\n");
-		tc_print_action(f, tb[TCA_FW_ACT]);
+		tc_print_action(f, tb[TCA_FW_ACT], 0);
 	}
 	return 0;
 }
diff --git a/tc/f_matchall.c b/tc/f_matchall.c
index 5a51e75..d78660e 100644
--- a/tc/f_matchall.c
+++ b/tc/f_matchall.c
@@ -145,7 +145,7 @@ static int matchall_print_opt(struct filter_util *qu, FILE *f,
 	}
 
 	if (tb[TCA_MATCHALL_ACT])
-		tc_print_action(f, tb[TCA_MATCHALL_ACT]);
+		tc_print_action(f, tb[TCA_MATCHALL_ACT], 0);
 
 	return 0;
 }
diff --git a/tc/f_route.c b/tc/f_route.c
index 30514c4..e88313f 100644
--- a/tc/f_route.c
+++ b/tc/f_route.c
@@ -168,7 +168,7 @@ static int route_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 	if (tb[TCA_ROUTE4_POLICE])
 		tc_print_police(f, tb[TCA_ROUTE4_POLICE]);
 	if (tb[TCA_ROUTE4_ACT])
-		tc_print_action(f, tb[TCA_ROUTE4_ACT]);
+		tc_print_action(f, tb[TCA_ROUTE4_ACT], 0);
 	return 0;
 }
 
diff --git a/tc/f_rsvp.c b/tc/f_rsvp.c
index 94bfbef..65caeb4 100644
--- a/tc/f_rsvp.c
+++ b/tc/f_rsvp.c
@@ -402,7 +402,7 @@ static int rsvp_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt, _
 	}
 
 	if (tb[TCA_RSVP_ACT]) {
-		tc_print_action(f, tb[TCA_RSVP_ACT]);
+		tc_print_action(f, tb[TCA_RSVP_ACT], 0);
 	}
 	if (tb[TCA_RSVP_POLICE])
 		tc_print_police(f, tb[TCA_RSVP_POLICE]);
diff --git a/tc/f_tcindex.c b/tc/f_tcindex.c
index 784c890..dd1cb47 100644
--- a/tc/f_tcindex.c
+++ b/tc/f_tcindex.c
@@ -173,7 +173,7 @@ static int tcindex_print_opt(struct filter_util *qu, FILE *f,
 	}
 	if (tb[TCA_TCINDEX_ACT]) {
 		fprintf(f, "\n");
-		tc_print_action(f, tb[TCA_TCINDEX_ACT]);
+		tc_print_action(f, tb[TCA_TCINDEX_ACT], 0);
 	}
 	return 0;
 }
diff --git a/tc/f_u32.c b/tc/f_u32.c
index ff700e9..4abb2fa 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1337,7 +1337,7 @@ static int u32_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt,
 	}
 
 	if (tb[TCA_U32_ACT])
-		tc_print_action(f, tb[TCA_U32_ACT]);
+		tc_print_action(f, tb[TCA_U32_ACT], 0);
 
 	return 0;
 }
diff --git a/tc/m_action.c b/tc/m_action.c
index 6ebe85e..88a377c 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -346,21 +346,24 @@ tc_print_action_flush(FILE *f, const struct rtattr *arg)
 }
 
 int
-tc_print_action(FILE *f, const struct rtattr *arg)
+tc_print_action(FILE *f, const struct rtattr *arg, unsigned short tot_acts)
 {
 
 	int i;
-	struct rtattr *tb[TCA_ACT_MAX_PRIO + 1];
 
 	if (arg == NULL)
 		return 0;
 
-	parse_rtattr_nested(tb, TCA_ACT_MAX_PRIO, arg);
+	if (!tot_acts)
+		tot_acts = TCA_ACT_MAX_PRIO;
+
+	struct rtattr *tb[tot_acts + 1];
+	parse_rtattr_nested(tb, tot_acts, arg);
 
 	if (tab_flush && NULL != tb[0]  && NULL == tb[1])
 		return tc_print_action_flush(f, tb[0]);
 
-	for (i = 0; i < TCA_ACT_MAX_PRIO; i++) {
+	for (i = 0; i < tot_acts; i++) {
 		if (tb[i]) {
 			fprintf(f, "\n\taction order %d: ", i);
 			if (tc_print_one_action(f, tb[i]) < 0) {
@@ -380,7 +383,8 @@ int print_action(const struct sockaddr_nl *who,
 	FILE *fp = (FILE *)arg;
 	struct tcamsg *t = NLMSG_DATA(n);
 	int len = n->nlmsg_len;
-	struct rtattr *tb[TCAA_MAX+1];
+	__u32 *tot_acts = NULL;
+	struct rtattr *tb[TCA_ROOT_MAX+1];
 
 	len -= NLMSG_LENGTH(sizeof(*t));
 
@@ -389,8 +393,12 @@ int print_action(const struct sockaddr_nl *who,
 		return -1;
 	}
 
-	parse_rtattr(tb, TCAA_MAX, TA_RTA(t), len);
+	parse_rtattr(tb, TCA_ROOT_MAX, TA_RTA(t), len);
+
+	if (tb[TCA_ROOT_COUNT])
+		tot_acts = RTA_DATA(tb[TCA_ROOT_COUNT]);
 
+	fprintf(fp, "total acts %d \n", tot_acts?*tot_acts:0);
 	if (tb[TCA_ACT_TAB] == NULL) {
 		if (n->nlmsg_type != RTM_GETACTION)
 			fprintf(stderr, "print_action: NULL kind\n");
@@ -414,7 +422,9 @@ int print_action(const struct sockaddr_nl *who,
 			fprintf(fp, "Replaced action ");
 		}
 	}
-	tc_print_action(fp, tb[TCA_ACT_TAB]);
+
+
+	tc_print_action(fp, tb[TCA_ACT_TAB], tot_acts?*tot_acts:0);
 
 	return 0;
 }
@@ -427,7 +437,7 @@ static int tc_action_gd(int cmd, unsigned int flags, int *argc_p, char ***argv_p
 	char **argv = *argv_p;
 	int prio = 0;
 	int ret = 0;
-	__u32 i;
+	__u32 i = 0;
 	struct rtattr *tail;
 	struct rtattr *tail2;
 	struct nlmsghdr *ans = NULL;
@@ -498,7 +508,8 @@ static int tc_action_gd(int cmd, unsigned int flags, int *argc_p, char ***argv_p
 		tail2 = NLMSG_TAIL(&req.n);
 		addattr_l(&req.n, MAX_MSG, ++prio, NULL, 0);
 		addattr_l(&req.n, MAX_MSG, TCA_ACT_KIND, k, strlen(k) + 1);
-		addattr32(&req.n, MAX_MSG, TCA_ACT_INDEX, i);
+		if (i > 0)
+			addattr32(&req.n, MAX_MSG, TCA_ACT_INDEX, i);
 		tail2->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail2;
 
 	}
@@ -561,12 +572,16 @@ static int tc_action_modify(int cmd, unsigned int flags, int *argc_p, char ***ar
 	return ret;
 }
 
-static int tc_act_list_or_flush(int argc, char **argv, int event)
+static int tc_act_list_or_flush(int *argc_p, char ***argv_p, int event)
 {
+	struct rtattr *tail, *tail2, *tail3, *tail4;
 	int ret = 0, prio = 0, msg_size = 0;
-	char k[16];
-	struct rtattr *tail, *tail2;
 	struct action_util *a = NULL;
+	struct __nla_bit_flags flag_select = { 0 };
+	char **argv = *argv_p;
+	__u32 msec_since = 0;
+	int argc = *argc_p;
+	char k[16];
 	struct {
 		struct nlmsghdr         n;
 		struct tcamsg           t;
@@ -597,11 +612,38 @@ static int tc_act_list_or_flush(int argc, char **argv, int event)
 	}
 	strncpy(k, *argv, sizeof(k) - 1);
 
+	argc -= 1;
+	argv += 1;
+
+	fprintf(stderr, "%d: <%s>\n", argc, *argv);
+
+	if (argc && (strcmp(*argv, "since") == 0)) {
+	    NEXT_ARG();
+	    if (get_u32(&msec_since, *argv, 0))
+		    invarg("dump time \"since\" is invalid", *argv);
+	}
+
 	addattr_l(&req.n, MAX_MSG, ++prio, NULL, 0);
 	addattr_l(&req.n, MAX_MSG, TCA_ACT_KIND, k, strlen(k) + 1);
 	tail2->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail2;
 	tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
 
+	tail3 = NLMSG_TAIL(&req.n);
+	flag_select.nla_flag_values |= TCA_FLAG_LARGE_DUMP_ON;
+	flag_select.nla_flag_selector |= TCA_FLAG_LARGE_DUMP_ON;
+#if 0
+	flag_select.nla_flag_values |= 8; /* test rejection */
+	flag_select.nla_flag_selector |= 8; /* test rejection */
+#endif
+	addattr_l(&req.n, MAX_MSG, TCA_ROOT_FLAGS, &flag_select,
+		  sizeof(struct __nla_bit_flags));
+	tail3->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail3;
+	if (msec_since) {
+		fprintf(stderr, "XXX: since %d\n", msec_since);
+		tail4 = NLMSG_TAIL(&req.n);
+		addattr32(&req.n, MAX_MSG, TCA_ROOT_TIME_DELTA, msec_since);
+		tail4->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail4;
+	}
 	msg_size = NLMSG_ALIGN(req.n.nlmsg_len) - NLMSG_ALIGN(sizeof(struct nlmsghdr));
 
 	if (event == RTM_GETACTION) {
@@ -626,6 +668,8 @@ static int tc_act_list_or_flush(int argc, char **argv, int event)
 
 bad_val:
 
+	*argc_p = argc;
+	*argv_p = argv;
 	return ret;
 }
 
@@ -655,13 +699,21 @@ int do_action(int argc, char **argv)
 				act_usage();
 				return -1;
 			}
-			return tc_act_list_or_flush(argc-2, argv+2, RTM_GETACTION);
+
+			argc -= 2;
+			argv += 2;
+			return tc_act_list_or_flush(&argc, &argv,
+						    RTM_GETACTION);
 		} else if (matches(*argv, "flush") == 0) {
 			if (argc <= 2) {
 				act_usage();
 				return -1;
 			}
-			return tc_act_list_or_flush(argc-2, argv+2, RTM_DELACTION);
+
+			argc -= 2;
+			argv += 2;
+			return tc_act_list_or_flush(&argc, &argv,
+						    RTM_DELACTION);
 		} else if (matches(*argv, "help") == 0) {
 			act_usage();
 			return -1;
diff --git a/tc/tc_util.h b/tc/tc_util.h
index 4db26c6..af578f9 100644
--- a/tc/tc_util.h
+++ b/tc/tc_util.h
@@ -106,7 +106,7 @@ int act_parse_police(struct action_util *a, int *argc_p,
 		     char ***argv_p, int tca_id, struct nlmsghdr *n);
 int print_police(struct action_util *a, FILE *f, struct rtattr *tb);
 int police_print_xstats(struct action_util *a, FILE *f, struct rtattr *tb);
-int tc_print_action(FILE *f, const struct rtattr *tb);
+int tc_print_action(FILE *f, const struct rtattr *tb, unsigned short tot_acts);
 int tc_print_ipt(FILE *f, const struct rtattr *tb);
 int parse_action(int *argc_p, char ***argv_p, int tca_id, struct nlmsghdr *n);
 void print_tm(FILE *f, const struct tcf_t *tm);

^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-11 11:53 ` [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS Jamal Hadi Salim
@ 2017-06-11 13:49   ` Jiri Pirko
  2017-06-11 17:38     ` Jamal Hadi Salim
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-11 13:49 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>Generic bitflags attribute content sent to the kernel by user.
>With this type the user can either set or unset a flag in the
>kernel.
>
>The nla_flag_values is a bitmap that defines the values being set
>The nla_flag_selector is a bitmask that defines which value is legit.
>
>A check is made to ensure the rules that a kernel subsystem always
>conforms to bitflags the kernel already knows about. i.e
>if the user tries to set a bit flag that is not understood then
>the _it will be rejected_.
>
>In the most basic form, the user specifies the attribute policy as:
>[ATTR_GOO] = { .type = NLA_FLAG_BITS, .validation_data = &myvalidflags },
>
>where myvalidflags is the bit mask of the flags the kernel understands.
>
>If the user _does not_ provide myvalidflags then the attribute will
>also be rejected.
>
>Examples:
>nla_flag_values = 0x0, and nla_flag_selector = 0x1
>implies we are selecting bit 1 and we want to set its value to 0.
>
>nla_flag_values = 0x2, and nla_flag_selector = 0x2
>implies we are selecting bit 2 and we want to set its value to 1.
>
>This patch also provides an extra feature: a validation callback
>that could be speaciliazed for other types.

s/speaciliazed/speciliazed/


>This feature is intended to be used by a kernel subsystem to check
>for a combination of bits being present. Example "bit x is valid
>only if bit y and z are present".
>
>So a kernel subsystem could specify validation rules of the following
>nature:
>
>[ATTR_GOO] = { .type = MYTYPE,
>	       .validation_data = &myvalidation_data,
>               .validate_content = mycontent_validator },

Indent is wrong. (Does not matter really in desc, but anyway)


>
>With validator callback looking like:
>
>int mycontent_validator(const struct nlattr *nla, void *valid_data)
>{
>       const struct myattribute *user_data = nla_data(nla);
>       struct myvalidation_struct *valid_data_constraint = valid_data;
>
>      ... return appropriate error code etc ...
>}
>
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Suggested-by: Jiri Pirko <jiri@mellanox.com>


>---
> include/net/netlink.h          | 11 +++++++++++
> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
> lib/nlattr.c                   | 25 +++++++++++++++++++++++++
> 3 files changed, 53 insertions(+)
>
>diff --git a/include/net/netlink.h b/include/net/netlink.h
>index 0170917..8ab9784 100644
>--- a/include/net/netlink.h
>+++ b/include/net/netlink.h
>@@ -6,6 +6,11 @@
> #include <linux/jiffies.h>
> #include <linux/in6.h>
> 
>+struct nla_bit_flags {
>+	u32 nla_flag_values;
>+	u32 nla_flag_selector;
>+};

I don't understand why you redefine the struct here. You already have it
defined in the uapi: struct __nla_bit_flags

Just move this (struct nla_bit_flags) to the uapi and remove
__nla_bit_flags ?



>+
> /* ========================================================================
>  *         Netlink Messages and Attributes Interface (As Seen On TV)
>  * ------------------------------------------------------------------------
>@@ -178,6 +183,7 @@ enum {
> 	NLA_S16,
> 	NLA_S32,
> 	NLA_S64,
>+	NLA_FLAG_BITS,
> 	__NLA_TYPE_MAX,
> };
> 
>@@ -206,6 +212,7 @@ enum {
>  *    NLA_MSECS            Leaving the length field zero will verify the
>  *                         given type fits, using it verifies minimum length
>  *                         just like "All other"
>+ *    NLA_FLAG_BITS        A bitmap/bitselector attribute
>  *    All other            Minimum length of attribute payload
>  *
>  * Example:
>@@ -213,11 +220,15 @@ enum {
>  * 	[ATTR_FOO] = { .type = NLA_U16 },
>  *	[ATTR_BAR] = { .type = NLA_STRING, .len = BARSIZ },
>  *	[ATTR_BAZ] = { .len = sizeof(struct mystruct) },
>+ *	[ATTR_GOO] = { .type = NLA_FLAG_BITS, .validation_data = &myvalidflags },
>  * };
>  */
> struct nla_policy {
> 	u16		type;
> 	u16		len;
>+	void            *validation_data;
>+	int             (*validate_content)(const struct nlattr *nla,
>+					    const void *validation_data);
> };
> 
> /**
>diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>index 564790e..8f07957 100644
>--- a/include/uapi/linux/rtnetlink.h
>+++ b/include/uapi/linux/rtnetlink.h
>@@ -179,6 +179,23 @@ struct rtattr {
> #define RTA_DATA(rta)   ((void*)(((char*)(rta)) + RTA_LENGTH(0)))
> #define RTA_PAYLOAD(rta) ((int)((rta)->rta_len) - RTA_LENGTH(0))
> 
>+/* Generic bitflags attribute content sent to the kernel.
>+ *
>+ * The nla_flag_values is a bitmap that defines the values being set
>+ * The nla_flag_selector is a bitmask that defines which value is legit
>+ *
>+ * Examples:
>+ *  nla_flag_values = 0x0, and nla_flag_selector = 0x1
>+ *  implies we are selecting bit 1 and we want to set its value to 0.
>+ *
>+ *  nla_flag_values = 0x2, and nla_flag_selector = 0x2
>+ *  implies we are selecting bit 2 and we want to set its value to 1.
>+ *
>+ */
>+struct __nla_bit_flags {
>+	__u32 nla_flag_values;
>+	__u32 nla_flag_selector;
>+};
> 
> 
> 
>diff --git a/lib/nlattr.c b/lib/nlattr.c
>index a7e0b16..78fed43 100644
>--- a/lib/nlattr.c
>+++ b/lib/nlattr.c
>@@ -27,6 +27,21 @@
> 	[NLA_S64]	= sizeof(s64),
> };
> 
>+static int validate_nla_bit_flags(const struct nlattr *nla, void *valid_data)
>+{
>+	const struct nla_bit_flags *nbf = nla_data(nla);
>+	u32 *valid_flags_mask = valid_data;
>+
>+	if (!valid_data)
>+		return -EINVAL;
>+
>+

Avoid one empty line here (you have 2)


>+	if (nbf->nla_flag_values & ~*valid_flags_mask)
>+		return -EINVAL;
>+
>+	return 0;
>+}
>+
> static int validate_nla(const struct nlattr *nla, int maxtype,
> 			const struct nla_policy *policy)
> {
>@@ -46,6 +61,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> 			return -ERANGE;
> 		break;
> 
>+	case NLA_FLAG_BITS:
>+		if (attrlen != 8) /* 2 x 32 bits */

sizeof(struct nla_bit_flags) instead of 8 please, you can skip the
comment then.


>+			return -ERANGE;
>+
>+		return validate_nla_bit_flags(nla, pt->validation_data);
>+		break;
>+
> 	case NLA_NUL_STRING:
> 		if (pt->len)
> 			minlen = min_t(int, attrlen, pt->len + 1);
>@@ -103,6 +125,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
> 			return -ERANGE;
> 	}
> 
>+	if (pt->validate_content)
>+		return pt->validate_content(nla, pt->validation_data);

This validation mechanism is completely independent from the added NLA_FLAG_BITS
attr as it could be used with other attribute types. Please have it as a
separate patch.


>+
> 	return 0;
> }
> 
>-- 
>1.9.1
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 2/4] net sched actions: Use proper root attribute table for actions
  2017-06-11 11:53 ` [PATCH net-next v10 2/4] net sched actions: Use proper root attribute table for actions Jamal Hadi Salim
@ 2017-06-11 13:55   ` Jiri Pirko
  0 siblings, 0 replies; 28+ messages in thread
From: Jiri Pirko @ 2017-06-11 13:55 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Sun, Jun 11, 2017 at 01:53:44PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>Bug fix for an issue which has been around for about a decade.
>We got away with it because the enumeration was larger than needed.
>
>Fixes: 7ba699c604ab ("[NET_SCHED]: Convert actions from rtnetlink to new netlink API")
>Suggested-by: Jiri Pirko <jiri@mellanox.com>
>Reviewed-by: Simon Horman <simon.horman@netronome.com>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Jiri Pirko <jiri@mellanox.com>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-11 11:53 ` [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch Jamal Hadi Salim
@ 2017-06-11 14:13   ` Jiri Pirko
  2017-06-11 17:45     ` Jamal Hadi Salim
  2017-06-12 11:16     ` Jamal Hadi Salim
  0 siblings, 2 replies; 28+ messages in thread
From: Jiri Pirko @ 2017-06-11 14:13 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Sun, Jun 11, 2017 at 01:53:45PM CEST, jhs@mojatatu.com wrote:
>From: Jamal Hadi Salim <jhs@mojatatu.com>
>
>When you dump hundreds of thousands of actions, getting only 32 per
>dump batch even when the socket buffer and memory allocations allow
>is inefficient.
>
>With this change, the user will get as many as possibly fitting
>within the given constraints available to the kernel.
>
>The top level action TLV space is extended. An attribute
>TCA_ROOT_FLAGS is used to carry flags; flag TCA_FLAG_LARGE_DUMP_ON
>is set by the user indicating the user is capable of processing
>these large dumps. Older user space which doesnt set this flag
>doesnt get the large (than 32) batches.
>The kernel uses the TCA_ROOT_COUNT attribute to tell the user how many
>actions are put in a single batch. As such user space app knows how long
>to iterate (independent of the type of action being dumped)
>instead of hardcoded maximum of 32 thus maintaining backward compat.
>
>Some results dumping 1.5M actions below:
>first an unpatched tc which doesnt understand these features...
>
>prompt$ time -p tc actions ls action gact | grep index | wc -l
>1500000
>real 1388.43
>user 2.07
>sys 1386.79
>
>Now lets see a patched tc which sets the correct flags when requesting
>a dump:
>
>prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>1500000
>real 178.13
>user 2.02
>sys 176.96
>
>That is about 8x performance improvement for tc app which sets its
>receive buffer to about 32K.
>
>Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>---
> include/uapi/linux/rtnetlink.h | 22 +++++++++++++++++--
> net/sched/act_api.c            | 50 +++++++++++++++++++++++++++++++++---------
> 2 files changed, 60 insertions(+), 12 deletions(-)
>
>diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>index 8f07957..7d2030f 100644
>--- a/include/uapi/linux/rtnetlink.h
>+++ b/include/uapi/linux/rtnetlink.h
>@@ -693,10 +693,28 @@ struct tcamsg {
> 	unsigned char	tca__pad1;
> 	unsigned short	tca__pad2;
> };
>+
>+enum {
>+	TCA_ROOT_UNSPEC,
>+	TCA_ROOT_TAB,
>+#define TCA_ACT_TAB TCA_ROOT_TAB
>+#define TCAA_MAX TCA_ROOT_TAB
>+	TCA_ROOT_FLAGS,
>+	TCA_ROOT_COUNT,
>+	__TCA_ROOT_MAX,
>+#define	TCA_ROOT_MAX (__TCA_ROOT_MAX - 1)
>+};
>+
> #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>-#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>-#define TCAA_MAX 1
>+/* tcamsg flags stored in attribute TCA_ROOT_FLAGS
>+ *
>+ * TCA_FLAG_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>+ * actions in a dump. All dump responses will contain the number of actions
>+ * being dumped stored in for user app's consumption in TCA_ROOT_COUNT
>+ *
>+ */
>+#define TCA_FLAG_LARGE_DUMP_ON		(1 << 0)
> 
> /* New extended info filters for IFLA_EXT_MASK */
> #define RTEXT_FILTER_VF		(1 << 0)
>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>index 400eb6e..935dc19 100644
>--- a/net/sched/act_api.c
>+++ b/net/sched/act_api.c
>@@ -110,6 +110,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> 			   struct netlink_callback *cb)
> {
> 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>+	u32 act_flags = cb->args[2];
> 	struct nlattr *nest;
> 
> 	spin_lock_bh(&hinfo->lock);
>@@ -138,14 +139,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
> 			}
> 			nla_nest_end(skb, nest);
> 			n_i++;
>-			if (n_i >= TCA_ACT_MAX_PRIO)
>+			if (!(act_flags & TCA_FLAG_LARGE_DUMP_ON) &&
>+			    n_i >= TCA_ACT_MAX_PRIO)
> 				goto done;
> 		}
> 	}
> done:
> 	spin_unlock_bh(&hinfo->lock);
>-	if (n_i)
>+	if (n_i) {
> 		cb->args[0] += n_i;
>+		if (act_flags & TCA_FLAG_LARGE_DUMP_ON)
>+			cb->args[1] = n_i;
>+	}
> 	return n_i;
> 
> nla_put_failure:
>@@ -1068,11 +1073,17 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
> 	return tcf_add_notify(net, n, &actions, portid);
> }
> 
>+static u32 allowed_flags = TCA_FLAG_LARGE_DUMP_ON;

An empty line would be nice. Also, since this is outside a function, some
context prefix would be nice:
static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;


>+static const struct nla_policy tcaa_policy[TCA_ROOT_MAX + 1] = {
>+	[TCA_ROOT_FLAGS] = { .type = NLA_FLAG_BITS,
>+			     .validation_data = &allowed_flags },
>+};
>+
> static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> 			 struct netlink_ext_ack *extack)
> {
> 	struct net *net = sock_net(skb->sk);
>-	struct nlattr *tca[TCAA_MAX + 1];
>+	struct nlattr *tca[TCA_ROOT_MAX + 1];
> 	u32 portid = skb ? NETLINK_CB(skb).portid : 0;
> 	int ret = 0, ovr = 0;
> 
>@@ -1080,7 +1091,7 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> 	    !netlink_capable(skb, CAP_NET_ADMIN))
> 		return -EPERM;
> 
>-	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCAA_MAX, NULL,
>+	ret = nlmsg_parse(n, sizeof(struct tcamsg), tca, TCA_ROOT_MAX, NULL,
> 			  extack);
> 	if (ret < 0)
> 		return ret;
>@@ -1121,16 +1132,12 @@ static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n,
> 	return ret;
> }
> 
>-static struct nlattr *find_dump_kind(const struct nlmsghdr *n)
>+static struct nlattr *find_dump_kind(struct nlattr **nla)
> {
> 	struct nlattr *tb1, *tb2[TCA_ACT_MAX + 1];
> 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
>-	struct nlattr *nla[TCAA_MAX + 1];
> 	struct nlattr *kind;
> 
>-	if (nlmsg_parse(n, sizeof(struct tcamsg), nla, TCAA_MAX,
>-			NULL, NULL) < 0)
>-		return NULL;
> 	tb1 = nla[TCA_ACT_TAB];
> 	if (tb1 == NULL)
> 		return NULL;
>@@ -1157,8 +1164,18 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> 	struct tc_action_ops *a_o;
> 	int ret = 0;
> 	struct tcamsg *t = (struct tcamsg *) nlmsg_data(cb->nlh);
>-	struct nlattr *kind = find_dump_kind(cb->nlh);
>+	struct __nla_bit_flags select_flags;
>+	struct nlattr *count_attr = NULL;
>+	struct nlattr *tb[TCA_ROOT_MAX + 1];
>+	struct nlattr *kind = NULL;
>+	u32 act_count = 0;
>+
>+	ret = nlmsg_parse(cb->nlh, sizeof(struct tcamsg), tb, TCA_ROOT_MAX,
>+			  tcaa_policy, NULL);
>+	if (ret < 0)
>+		return ret;
> 
>+	kind = find_dump_kind(tb);
> 	if (kind == NULL) {
> 		pr_info("tc_dump_action: action bad kind\n");
> 		return 0;
>@@ -1168,14 +1185,24 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> 	if (a_o == NULL)
> 		return 0;
> 
>+	if (tb[TCA_ROOT_FLAGS])
>+		nla_memcpy(&select_flags, tb[TCA_ROOT_FLAGS],
>+			   sizeof(select_flags));

Please introduce a helper for this attr type in patch 1:

u32 select_flags;

select_flags = nla_get_flag_bits_values(tb[TCA_ROOT_FLAGS])


>+
> 	nlh = nlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> 			cb->nlh->nlmsg_type, sizeof(*t), 0);
> 	if (!nlh)
> 		goto out_module_put;
>+
>+	cb->args[2] = select_flags.nla_flag_values;
> 	t = nlmsg_data(nlh);
> 	t->tca_family = AF_UNSPEC;
> 	t->tca__pad1 = 0;
> 	t->tca__pad2 = 0;
>+	count_attr = nla_reserve(skb, TCA_ROOT_COUNT, sizeof(u32));
>+	if (!count_attr)
>+		goto out_module_put;
>+
> 
> 	nest = nla_nest_start(skb, TCA_ACT_TAB);
> 	if (nest == NULL)
>@@ -1188,6 +1215,9 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
> 	if (ret > 0) {
> 		nla_nest_end(skb, nest);
> 		ret = skb->len;
>+		act_count = cb->args[1];
>+		memcpy(nla_data(count_attr), &act_count, sizeof(u32));
>+		cb->args[1] = 0;
> 	} else
> 		nlmsg_trim(skb, b);
> 
>-- 
>1.9.1
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-11 13:49   ` Jiri Pirko
@ 2017-06-11 17:38     ` Jamal Hadi Salim
  2017-06-11 18:37       ` Jamal Hadi Salim
  0 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 17:38 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-11 09:49 AM, Jiri Pirko wrote:
> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>


>> This patch also provides an extra feature: a validation callback
>> that could be speaciliazed for other types.
> 
> s/speaciliazed/speciliazed/
> 

Will fix.


>>
>> [ATTR_GOO] = { .type = MYTYPE,
>> 	       .validation_data = &myvalidation_data,
>>                .validate_content = mycontent_validator },
> 
> Indent is wrong. (Does not matter really in desc, but anyway)
>

I cant find out how it got indented that way; my source
or email dont show it as such (but really doesnt matter).


> Suggested-by: Jiri Pirko <jiri@mellanox.com>
> 

Will add.

> 
>> ---
>> include/net/netlink.h          | 11 +++++++++++
>> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
>> lib/nlattr.c                   | 25 +++++++++++++++++++++++++
>> 3 files changed, 53 insertions(+)
>>
>> diff --git a/include/net/netlink.h b/include/net/netlink.h
>> index 0170917..8ab9784 100644
>> --- a/include/net/netlink.h
>> +++ b/include/net/netlink.h
>> @@ -6,6 +6,11 @@
>> #include <linux/jiffies.h>
>> #include <linux/in6.h>
>>
>> +struct nla_bit_flags {
>> +	u32 nla_flag_values;
>> +	u32 nla_flag_selector;
>> +};
> 
> I don't understand why you redefine the struct here. You already have it
> defined in the uapi: struct __nla_bit_flags
> 
> Just move this (struct nla_bit_flags) to the uapi and remove
> __nla_bit_flags ?
>

I am not sure that will compile since the type is defined in netlink.h
Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty
common approach; i will try to move it to uapi and keep that uapi
format. If it doesnt compile without acrobatics I will keep it as is.

> 
> 
>> +



>> diff --git a/lib/nlattr.c b/lib/nlattr.c
>> index a7e0b16..78fed43 100644
>> --- a/lib/nlattr.c
>> +++ b/lib/nlattr.c
>> @@ -27,6 +27,21 @@
>> 	[NLA_S64]	= sizeof(s64),
>> };
>>
>> +static int validate_nla_bit_flags(const struct nlattr *nla, void *valid_data)
>> +{
>> +	const struct nla_bit_flags *nbf = nla_data(nla);
>> +	u32 *valid_flags_mask = valid_data;
>> +
>> +	if (!valid_data)
>> +		return -EINVAL;
>> +
>> +
> 
> Avoid one empty line here (you have 2)
> 

Will fix.

> 
>> +	if (nbf->nla_flag_values & ~*valid_flags_mask)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> static int validate_nla(const struct nlattr *nla, int maxtype,
>> 			const struct nla_policy *policy)
>> {
>> @@ -46,6 +61,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>> 			return -ERANGE;
>> 		break;
>>
>> +	case NLA_FLAG_BITS:
>> +		if (attrlen != 8) /* 2 x 32 bits */
> 
> sizeof(struct nla_bit_flags) instead of 8 please, you can skip the
> comment then.
> 

Good point.

> 
>> +			return -ERANGE;
>> +
>> +		return validate_nla_bit_flags(nla, pt->validation_data);
>> +		break;
>> +
>> 	case NLA_NUL_STRING:
>> 		if (pt->len)
>> 			minlen = min_t(int, attrlen, pt->len + 1);
>> @@ -103,6 +125,9 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
>> 			return -ERANGE;
>> 	}
>>
>> +	if (pt->validate_content)
>> +		return pt->validate_content(nla, pt->validation_data);
> 
> This validation mechanism is completely independent from the added NLA_FLAG_BITS
> attr as it could be used with other attribute types. Please have it as a
> separate patch.
>


Ok - so one more patch then ;->

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-11 14:13   ` Jiri Pirko
@ 2017-06-11 17:45     ` Jamal Hadi Salim
  2017-06-12 11:16     ` Jamal Hadi Salim
  1 sibling, 0 replies; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 17:45 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-11 10:13 AM, Jiri Pirko wrote:
> Sun, Jun 11, 2017 at 01:53:45PM CEST, jhs@mojatatu.com wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> When you dump hundreds of thousands of actions, getting only 32 per
>> dump batch even when the socket buffer and memory allocations allow
>> is inefficient.
>>
>> With this change, the user will get as many as possibly fitting
>> within the given constraints available to the kernel.
>>
>> The top level action TLV space is extended. An attribute
>> TCA_ROOT_FLAGS is used to carry flags; flag TCA_FLAG_LARGE_DUMP_ON
>> is set by the user indicating the user is capable of processing
>> these large dumps. Older user space which doesnt set this flag
>> doesnt get the large (than 32) batches.
>> The kernel uses the TCA_ROOT_COUNT attribute to tell the user how many
>> actions are put in a single batch. As such user space app knows how long
>> to iterate (independent of the type of action being dumped)
>> instead of hardcoded maximum of 32 thus maintaining backward compat.
>>
>> Some results dumping 1.5M actions below:
>> first an unpatched tc which doesnt understand these features...
>>
>> prompt$ time -p tc actions ls action gact | grep index | wc -l
>> 1500000
>> real 1388.43
>> user 2.07
>> sys 1386.79
>>
>> Now lets see a patched tc which sets the correct flags when requesting
>> a dump:
>>
>> prompt$ time -p updatedtc actions ls action gact | grep index | wc -l
>> 1500000
>> real 178.13
>> user 2.02
>> sys 176.96
>>
>> That is about 8x performance improvement for tc app which sets its
>> receive buffer to about 32K.
>>
>> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
>> ---
>> include/uapi/linux/rtnetlink.h | 22 +++++++++++++++++--
>> net/sched/act_api.c            | 50 +++++++++++++++++++++++++++++++++---------
>> 2 files changed, 60 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
>> index 8f07957..7d2030f 100644
>> --- a/include/uapi/linux/rtnetlink.h
>> +++ b/include/uapi/linux/rtnetlink.h
>> @@ -693,10 +693,28 @@ struct tcamsg {
>> 	unsigned char	tca__pad1;
>> 	unsigned short	tca__pad2;
>> };
>> +
>> +enum {
>> +	TCA_ROOT_UNSPEC,
>> +	TCA_ROOT_TAB,
>> +#define TCA_ACT_TAB TCA_ROOT_TAB
>> +#define TCAA_MAX TCA_ROOT_TAB
>> +	TCA_ROOT_FLAGS,
>> +	TCA_ROOT_COUNT,
>> +	__TCA_ROOT_MAX,
>> +#define	TCA_ROOT_MAX (__TCA_ROOT_MAX - 1)
>> +};
>> +
>> #define TA_RTA(r)  ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct tcamsg))))
>> #define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
>> -#define TCA_ACT_TAB 1 /* attr type must be >=1 */	
>> -#define TCAA_MAX 1
>> +/* tcamsg flags stored in attribute TCA_ROOT_FLAGS
>> + *
>> + * TCA_FLAG_LARGE_DUMP_ON user->kernel to request for larger than TCA_ACT_MAX_PRIO
>> + * actions in a dump. All dump responses will contain the number of actions
>> + * being dumped stored in for user app's consumption in TCA_ROOT_COUNT
>> + *
>> + */
>> +#define TCA_FLAG_LARGE_DUMP_ON		(1 << 0)
>>
>> /* New extended info filters for IFLA_EXT_MASK */
>> #define RTEXT_FILTER_VF		(1 << 0)
>> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>> index 400eb6e..935dc19 100644
>> --- a/net/sched/act_api.c
>> +++ b/net/sched/act_api.c
>> @@ -110,6 +110,7 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> 			   struct netlink_callback *cb)
>> {
>> 	int err = 0, index = -1, i = 0, s_i = 0, n_i = 0;
>> +	u32 act_flags = cb->args[2];
>> 	struct nlattr *nest;
>>
>> 	spin_lock_bh(&hinfo->lock);
>> @@ -138,14 +139,18 @@ static int tcf_dump_walker(struct tcf_hashinfo *hinfo, struct sk_buff *skb,
>> 			}
>> 			nla_nest_end(skb, nest);
>> 			n_i++;
>> -			if (n_i >= TCA_ACT_MAX_PRIO)
>> +			if (!(act_flags & TCA_FLAG_LARGE_DUMP_ON) &&
>> +			    n_i >= TCA_ACT_MAX_PRIO)
>> 				goto done;
>> 		}
>> 	}
>> done:
>> 	spin_unlock_bh(&hinfo->lock);
>> -	if (n_i)
>> +	if (n_i) {
>> 		cb->args[0] += n_i;
>> +		if (act_flags & TCA_FLAG_LARGE_DUMP_ON)
>> +			cb->args[1] = n_i;
>> +	}
>> 	return n_i;
>>
>> nla_put_failure:
>> @@ -1068,11 +1073,17 @@ static int tcf_action_add(struct net *net, struct nlattr *nla,
>> 	return tcf_add_notify(net, n, &actions, portid);
>> }
>>
>> +static u32 allowed_flags = TCA_FLAG_LARGE_DUMP_ON;
> 
> An empty line would be nice. Also, since this is outside a function, some
> context prefix would be nice:
> static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON;
>

You are going to make me exceed the 80 char limit? ;->


>> +	if (tb[TCA_ROOT_FLAGS])
>> +		nla_memcpy(&select_flags, tb[TCA_ROOT_FLAGS],
>> +			   sizeof(select_flags));
> 
> Please introduce a helper for this attr type in patch 1:
> 
> u32 select_flags;
> 
> select_flags = nla_get_flag_bits_values(tb[TCA_ROOT_FLAGS])
> 

Sure.

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-11 17:38     ` Jamal Hadi Salim
@ 2017-06-11 18:37       ` Jamal Hadi Salim
  2017-06-12 10:34         ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-11 18:37 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-11 01:38 PM, Jamal Hadi Salim wrote:
> On 17-06-11 09:49 AM, Jiri Pirko wrote:
>> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> 
>>> This patch also provides an extra feature: a validation callback
>>> that could be speaciliazed for other types.
>>
>> s/speaciliazed/speciliazed/
>>
> 
> Will fix.
> 
> 
>>>
>>> [ATTR_GOO] = { .type = MYTYPE,
>>>            .validation_data = &myvalidation_data,
>>>                .validate_content = mycontent_validator },
>>
>> Indent is wrong. (Does not matter really in desc, but anyway)
>>
> 
> I cant find out how it got indented that way; my source
> or email dont show it as such (but really doesnt matter).
> 
> 
>> Suggested-by: Jiri Pirko <jiri@mellanox.com>
>>
> 
> Will add.
> 
>>
>>> ---
>>> include/net/netlink.h          | 11 +++++++++++
>>> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
>>> lib/nlattr.c                   | 25 +++++++++++++++++++++++++
>>> 3 files changed, 53 insertions(+)
>>>
>>> diff --git a/include/net/netlink.h b/include/net/netlink.h
>>> index 0170917..8ab9784 100644
>>> --- a/include/net/netlink.h
>>> +++ b/include/net/netlink.h
>>> @@ -6,6 +6,11 @@
>>> #include <linux/jiffies.h>
>>> #include <linux/in6.h>
>>>
>>> +struct nla_bit_flags {
>>> +    u32 nla_flag_values;
>>> +    u32 nla_flag_selector;
>>> +};
>>
>> I don't understand why you redefine the struct here. You already have it
>> defined in the uapi: struct __nla_bit_flags
>>
>> Just move this (struct nla_bit_flags) to the uapi and remove
>> __nla_bit_flags ?
>>
> 
> I am not sure that will compile since the type is defined in netlink.h
> Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty
> common approach; i will try to move it to uapi and keep that uapi
> format. If it doesnt compile without acrobatics I will keep it as is.
>

It doesnt compile - I could move it to linux/netlink.h but it seems
so out of place.
so i will keep things as is for now unless you can think of something
else.

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-11 18:37       ` Jamal Hadi Salim
@ 2017-06-12 10:34         ` Jiri Pirko
  2017-06-12 11:10           ` Jamal Hadi Salim
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-12 10:34 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote:
>On 17-06-11 01:38 PM, Jamal Hadi Salim wrote:
>> On 17-06-11 09:49 AM, Jiri Pirko wrote:
>> > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>> > > From: Jamal Hadi Salim <jhs@mojatatu.com>
>> 
>> 
>> > > This patch also provides an extra feature: a validation callback
>> > > that could be speaciliazed for other types.
>> > 
>> > s/speaciliazed/speciliazed/
>> > 
>> 
>> Will fix.
>> 
>> 
>> > > 
>> > > [ATTR_GOO] = { .type = MYTYPE,
>> > >            .validation_data = &myvalidation_data,
>> > >                .validate_content = mycontent_validator },
>> > 
>> > Indent is wrong. (Does not matter really in desc, but anyway)
>> > 
>> 
>> I cant find out how it got indented that way; my source
>> or email dont show it as such (but really doesnt matter).
>> 
>> 
>> > Suggested-by: Jiri Pirko <jiri@mellanox.com>
>> > 
>> 
>> Will add.
>> 
>> > 
>> > > ---
>> > > include/net/netlink.h          | 11 +++++++++++
>> > > include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
>> > > lib/nlattr.c                   | 25 +++++++++++++++++++++++++
>> > > 3 files changed, 53 insertions(+)
>> > > 
>> > > diff --git a/include/net/netlink.h b/include/net/netlink.h
>> > > index 0170917..8ab9784 100644
>> > > --- a/include/net/netlink.h
>> > > +++ b/include/net/netlink.h
>> > > @@ -6,6 +6,11 @@
>> > > #include <linux/jiffies.h>
>> > > #include <linux/in6.h>
>> > > 
>> > > +struct nla_bit_flags {
>> > > +    u32 nla_flag_values;
>> > > +    u32 nla_flag_selector;
>> > > +};
>> > 
>> > I don't understand why you redefine the struct here. You already have it
>> > defined in the uapi: struct __nla_bit_flags
>> > 
>> > Just move this (struct nla_bit_flags) to the uapi and remove
>> > __nla_bit_flags ?
>> > 
>> 
>> I am not sure that will compile since the type is defined in netlink.h
>> Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty
>> common approach; i will try to move it to uapi and keep that uapi
>> format. If it doesnt compile without acrobatics I will keep it as is.
>> 
>
>It doesnt compile - I could move it to linux/netlink.h but it seems
>so out of place.
>so i will keep things as is for now unless you can think of something
>else.

First of all, makes no sense to put this struct "struct __nla_bit_flags"
into rtnetlink.h uapi file. This is generic netlink stuff, not specifict
to rtnetlink.

I believe that this struct should go into:
include/uapi/linux/netlink.h

struct nla_flag_bits {
	__u32 nla_flag_bits_values;
	__u32 nla_flag_bits_selector;
};

Then you can use it from userspace and everywhere in kernel.

Btw, I find it very odd that enum containling NLA_* like NLA_U32 and
others is not part of uapi file and is rather defined in
include/net/netlink.h. Any idea why?

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 10:34         ` Jiri Pirko
@ 2017-06-12 11:10           ` Jamal Hadi Salim
  2017-06-12 11:43             ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-12 11:10 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-12 06:34 AM, Jiri Pirko wrote:
> Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote:
>> On 17-06-11 01:38 PM, Jamal Hadi Salim wrote:
>>> On 17-06-11 09:49 AM, Jiri Pirko wrote:
>>>> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>>>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>>
>>>
>>>>> This patch also provides an extra feature: a validation callback
>>>>> that could be speaciliazed for other types.
>>>>
>>>> s/speaciliazed/speciliazed/
>>>>
>>>
>>> Will fix.
>>>
>>>
>>>>>
>>>>> [ATTR_GOO] = { .type = MYTYPE,
>>>>>             .validation_data = &myvalidation_data,
>>>>>                 .validate_content = mycontent_validator },
>>>>
>>>> Indent is wrong. (Does not matter really in desc, but anyway)
>>>>
>>>
>>> I cant find out how it got indented that way; my source
>>> or email dont show it as such (but really doesnt matter).
>>>
>>>
>>>> Suggested-by: Jiri Pirko <jiri@mellanox.com>
>>>>
>>>
>>> Will add.
>>>
>>>>
>>>>> ---
>>>>> include/net/netlink.h          | 11 +++++++++++
>>>>> include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
>>>>> lib/nlattr.c                   | 25 +++++++++++++++++++++++++
>>>>> 3 files changed, 53 insertions(+)
>>>>>
>>>>> diff --git a/include/net/netlink.h b/include/net/netlink.h
>>>>> index 0170917..8ab9784 100644
>>>>> --- a/include/net/netlink.h
>>>>> +++ b/include/net/netlink.h
>>>>> @@ -6,6 +6,11 @@
>>>>> #include <linux/jiffies.h>
>>>>> #include <linux/in6.h>
>>>>>
>>>>> +struct nla_bit_flags {
>>>>> +    u32 nla_flag_values;
>>>>> +    u32 nla_flag_selector;
>>>>> +};
>>>>
>>>> I don't understand why you redefine the struct here. You already have it
>>>> defined in the uapi: struct __nla_bit_flags
>>>>
>>>> Just move this (struct nla_bit_flags) to the uapi and remove
>>>> __nla_bit_flags ?
>>>>
>>>
>>> I am not sure that will compile since the type is defined in netlink.h
>>> Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty
>>> common approach; i will try to move it to uapi and keep that uapi
>>> format. If it doesnt compile without acrobatics I will keep it as is.
>>>
>>
>> It doesnt compile - I could move it to linux/netlink.h but it seems
>> so out of place.
>> so i will keep things as is for now unless you can think of something
>> else.
> 
> First of all, makes no sense to put this struct "struct __nla_bit_flags"
> into rtnetlink.h uapi file. This is generic netlink stuff, not specifict
> to rtnetlink.
> 
> I believe that this struct should go into:
> include/uapi/linux/netlink.h
> 
> struct nla_flag_bits {
> 	__u32 nla_flag_bits_values;
> 	__u32 nla_flag_bits_selector;
> };
> 
> Then you can use it from userspace and everywhere in kernel.
> 
That file seems to be very out of place for this stuff.

> Btw, I find it very odd that enum containling NLA_* like NLA_U32 and
> others is not part of uapi file and is rather defined in
> include/net/netlink.h. Any idea why?
> 

NLA_XXX are kernel side types.  They are part of net/netlink.h which is
not uapi accessible.
David Ahern has submitted a patch to move all those defines to iproute2.
Will make sense to move these to a uapi/linux/netlink-types.h but that
is waay beyond the scope of this patch set.

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-11 14:13   ` Jiri Pirko
  2017-06-11 17:45     ` Jamal Hadi Salim
@ 2017-06-12 11:16     ` Jamal Hadi Salim
  2017-06-12 11:47       ` Jiri Pirko
  1 sibling, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-12 11:16 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-11 10:13 AM, Jiri Pirko wrote:
> Sun, Jun 11, 2017 at 01:53:45PM CEST, jhs@mojatatu.com wrote:
[..]
>> @@ -1168,14 +1185,24 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>> 	if (a_o == NULL)
>> 		return 0;
>>
>> +	if (tb[TCA_ROOT_FLAGS])
>> +		nla_memcpy(&select_flags, tb[TCA_ROOT_FLAGS],
>> +			   sizeof(select_flags));
> 
> Please introduce a helper for this attr type in patch 1:
> 
> u32 select_flags;
> 
> select_flags = nla_get_flag_bits_values(tb[TCA_ROOT_FLAGS])
> 

This also is not useful.
It happens to be ok for this use case but not for the
general case. i.e.
We need to get the whole struct not just the values
and use the selector to pick what bits are affected.
Example if bit X is set to 1 in selector and bit X in value
is 0, then we set the kernel's bit X to 0.

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 11:10           ` Jamal Hadi Salim
@ 2017-06-12 11:43             ` Jiri Pirko
  2017-06-12 13:51               ` Jamal Hadi Salim
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-12 11:43 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Mon, Jun 12, 2017 at 01:10:56PM CEST, jhs@mojatatu.com wrote:
>On 17-06-12 06:34 AM, Jiri Pirko wrote:
>> Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote:
>> > On 17-06-11 01:38 PM, Jamal Hadi Salim wrote:
>> > > On 17-06-11 09:49 AM, Jiri Pirko wrote:
>> > > > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>> > > > > From: Jamal Hadi Salim <jhs@mojatatu.com>
>> > > 
>> > > 
>> > > > > This patch also provides an extra feature: a validation callback
>> > > > > that could be speaciliazed for other types.
>> > > > 
>> > > > s/speaciliazed/speciliazed/
>> > > > 
>> > > 
>> > > Will fix.
>> > > 
>> > > 
>> > > > > 
>> > > > > [ATTR_GOO] = { .type = MYTYPE,
>> > > > >             .validation_data = &myvalidation_data,
>> > > > >                 .validate_content = mycontent_validator },
>> > > > 
>> > > > Indent is wrong. (Does not matter really in desc, but anyway)
>> > > > 
>> > > 
>> > > I cant find out how it got indented that way; my source
>> > > or email dont show it as such (but really doesnt matter).
>> > > 
>> > > 
>> > > > Suggested-by: Jiri Pirko <jiri@mellanox.com>
>> > > > 
>> > > 
>> > > Will add.
>> > > 
>> > > > 
>> > > > > ---
>> > > > > include/net/netlink.h          | 11 +++++++++++
>> > > > > include/uapi/linux/rtnetlink.h | 17 +++++++++++++++++
>> > > > > lib/nlattr.c                   | 25 +++++++++++++++++++++++++
>> > > > > 3 files changed, 53 insertions(+)
>> > > > > 
>> > > > > diff --git a/include/net/netlink.h b/include/net/netlink.h
>> > > > > index 0170917..8ab9784 100644
>> > > > > --- a/include/net/netlink.h
>> > > > > +++ b/include/net/netlink.h
>> > > > > @@ -6,6 +6,11 @@
>> > > > > #include <linux/jiffies.h>
>> > > > > #include <linux/in6.h>
>> > > > > 
>> > > > > +struct nla_bit_flags {
>> > > > > +    u32 nla_flag_values;
>> > > > > +    u32 nla_flag_selector;
>> > > > > +};
>> > > > 
>> > > > I don't understand why you redefine the struct here. You already have it
>> > > > defined in the uapi: struct __nla_bit_flags
>> > > > 
>> > > > Just move this (struct nla_bit_flags) to the uapi and remove
>> > > > __nla_bit_flags ?
>> > > > 
>> > > 
>> > > I am not sure that will compile since the type is defined in netlink.h
>> > > Also, note: uapi uses _u32 and kernel uses u32 as types i.e it is pretty
>> > > common approach; i will try to move it to uapi and keep that uapi
>> > > format. If it doesnt compile without acrobatics I will keep it as is.
>> > > 
>> > 
>> > It doesnt compile - I could move it to linux/netlink.h but it seems
>> > so out of place.
>> > so i will keep things as is for now unless you can think of something
>> > else.
>> 
>> First of all, makes no sense to put this struct "struct __nla_bit_flags"
>> into rtnetlink.h uapi file. This is generic netlink stuff, not specifict
>> to rtnetlink.
>> 
>> I believe that this struct should go into:
>> include/uapi/linux/netlink.h
>> 
>> struct nla_flag_bits {
>> 	__u32 nla_flag_bits_values;
>> 	__u32 nla_flag_bits_selector;
>> };
>> 
>> Then you can use it from userspace and everywhere in kernel.
>> 
>That file seems to be very out of place for this stuff.

Howcome? It is a common netlink api.


>
>> Btw, I find it very odd that enum containling NLA_* like NLA_U32 and
>> others is not part of uapi file and is rather defined in
>> include/net/netlink.h. Any idea why?
>> 
>
>NLA_XXX are kernel side types.  They are part of net/netlink.h which is
>not uapi accessible.
>David Ahern has submitted a patch to move all those defines to iproute2.
>Will make sense to move these to a uapi/linux/netlink-types.h but that
>is waay beyond the scope of this patch set.

Well, I don't think so :)

The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS
enum value. They should be in the same uapi file. That makes sense to me.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-12 11:16     ` Jamal Hadi Salim
@ 2017-06-12 11:47       ` Jiri Pirko
  2017-06-12 13:57         ` Jamal Hadi Salim
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-12 11:47 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Mon, Jun 12, 2017 at 01:16:05PM CEST, jhs@mojatatu.com wrote:
>On 17-06-11 10:13 AM, Jiri Pirko wrote:
>> Sun, Jun 11, 2017 at 01:53:45PM CEST, jhs@mojatatu.com wrote:
>[..]
>> > @@ -1168,14 +1185,24 @@ static int tc_dump_action(struct sk_buff *skb, struct netlink_callback *cb)
>> > 	if (a_o == NULL)
>> > 		return 0;
>> > 
>> > +	if (tb[TCA_ROOT_FLAGS])
>> > +		nla_memcpy(&select_flags, tb[TCA_ROOT_FLAGS],
>> > +			   sizeof(select_flags));
>> 
>> Please introduce a helper for this attr type in patch 1:
>> 
>> u32 select_flags;
>> 
>> select_flags = nla_get_flag_bits_values(tb[TCA_ROOT_FLAGS])
>> 
>
>This also is not useful.
>It happens to be ok for this use case but not for the
>general case. i.e.
>We need to get the whole struct not just the values
>and use the selector to pick what bits are affected.
>Example if bit X is set to 1 in selector and bit X in value
>is 0, then we set the kernel's bit X to 0.

Sure, have another helper for selector then.
Or, you can have:

	struct nla_flag_bits *fb;
	fb = nla_get_flag_bits(tb[TCA_ROOT_FLAGS]);

Or all 3 helpers. My point is, it is a specific netlink attribute with
specific format, it should have get/put helpers.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 11:43             ` Jiri Pirko
@ 2017-06-12 13:51               ` Jamal Hadi Salim
  2017-06-12 14:14                 ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-12 13:51 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-12 07:43 AM, Jiri Pirko wrote:
> Mon, Jun 12, 2017 at 01:10:56PM CEST, jhs@mojatatu.com wrote:
>> On 17-06-12 06:34 AM, Jiri Pirko wrote:
>>> Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote:
>>>> On 17-06-11 01:38 PM, Jamal Hadi Salim wrote:
>>>>> On 17-06-11 09:49 AM, Jiri Pirko wrote:
>>>>>> Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>>>>>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>>>>
>>>>>
>>>>>>> This patch also provides an extra feature: a validation callback
>>>>>>> that could be speaciliazed for other types.
>>>>>>
>>>>>> s/speaciliazed/speciliazed/
>>>>>>
>>>>>
>>>>> Will fix.
>>>>>
>>>>>
>>>>>>>
>>>>>>> [ATTR_GOO] = { .type = MYTYPE,
>>>>>>>              .validation_data = &myvalidation_data,
>>>>>>>                  .validate_content = mycontent_validator },
>>>>>>
>>>>>> Indent is wrong. (Does not matter really in desc, but anyway)
>>>>>>
>>>>>
>>>>> I cant find out how it got indented that way; my source
>>>>> or email dont show it as such (but really doesnt matter).
>>>>>
>>>>>
>>>>>> Suggested-by: Jiri Pirko <jiri@mellanox.com>
>>>>>>
>>>>>
>>>>> Will add.
>>>>>
>>>>>>

>>> I believe that this struct should go into:
>>> include/uapi/linux/netlink.h
>>>
>>> struct nla_flag_bits {
>>> 	__u32 nla_flag_bits_values;
>>> 	__u32 nla_flag_bits_selector;
>>> };
>>>
>>> Then you can use it from userspace and everywhere in kernel.
>>>
>> That file seems to be very out of place for this stuff.
> 
> Howcome? It is a common netlink api.
> 

Take a look at that file's content. It talks about what goes in
the netlink header. Adding types in it seems out of place.


>> NLA_XXX are kernel side types.  They are part of net/netlink.h which is
>> not uapi accessible.
>> David Ahern has submitted a patch to move all those defines to iproute2.
>> Will make sense to move these to a uapi/linux/netlink-types.h but that
>> is waay beyond the scope of this patch set.
> 
> Well, I don't think so :)
> 
> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS
> enum value. They should be in the same uapi file. That makes sense to me.
> 

Sure - they should be in the same file. But is it uapi/linux/netlink.h?

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-12 11:47       ` Jiri Pirko
@ 2017-06-12 13:57         ` Jamal Hadi Salim
  2017-06-12 14:16           ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-12 13:57 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-12 07:47 AM, Jiri Pirko wrote:
> Mon, Jun 12, 2017 at 01:16:05PM CEST, jhs@mojatatu.com wrote:
>> On 17-06-11 10:13 AM, Jiri Pirko wrote:

>> This also is not useful.
>> It happens to be ok for this use case but not for the
>> general case. i.e.
>> We need to get the whole struct not just the values
>> and use the selector to pick what bits are affected.
>> Example if bit X is set to 1 in selector and bit X in value
>> is 0, then we set the kernel's bit X to 0.
> 
> Sure, have another helper for selector then.
> Or, you can have:
> 
> 	struct nla_flag_bits *fb;
> 	fb = nla_get_flag_bits(tb[TCA_ROOT_FLAGS]);
> 
> Or all 3 helpers. My point is, it is a specific netlink attribute with
> specific format, it should have get/put helpers.
> 

I did try nla_get_flag_bits(tb[TCA_ROOT_FLAGS], &fb) so i dont have
to do alloc/free - it ended being a single line function which
does a memcpy.
Is this really necessary?
The user of this structure better know it to make use of it.

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 13:51               ` Jamal Hadi Salim
@ 2017-06-12 14:14                 ` Jiri Pirko
  2017-06-12 15:00                   ` David Ahern
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-12 14:14 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv, dsahern

Mon, Jun 12, 2017 at 03:51:19PM CEST, jhs@mojatatu.com wrote:
>On 17-06-12 07:43 AM, Jiri Pirko wrote:
>> Mon, Jun 12, 2017 at 01:10:56PM CEST, jhs@mojatatu.com wrote:
>> > On 17-06-12 06:34 AM, Jiri Pirko wrote:
>> > > Sun, Jun 11, 2017 at 08:37:25PM CEST, jhs@mojatatu.com wrote:
>> > > > On 17-06-11 01:38 PM, Jamal Hadi Salim wrote:
>> > > > > On 17-06-11 09:49 AM, Jiri Pirko wrote:
>> > > > > > Sun, Jun 11, 2017 at 01:53:43PM CEST, jhs@mojatatu.com wrote:
>> > > > > > > From: Jamal Hadi Salim <jhs@mojatatu.com>
>> > > > > 
>> > > > > 
>> > > > > > > This patch also provides an extra feature: a validation callback
>> > > > > > > that could be speaciliazed for other types.
>> > > > > > 
>> > > > > > s/speaciliazed/speciliazed/
>> > > > > > 
>> > > > > 
>> > > > > Will fix.
>> > > > > 
>> > > > > 
>> > > > > > > 
>> > > > > > > [ATTR_GOO] = { .type = MYTYPE,
>> > > > > > >              .validation_data = &myvalidation_data,
>> > > > > > >                  .validate_content = mycontent_validator },
>> > > > > > 
>> > > > > > Indent is wrong. (Does not matter really in desc, but anyway)
>> > > > > > 
>> > > > > 
>> > > > > I cant find out how it got indented that way; my source
>> > > > > or email dont show it as such (but really doesnt matter).
>> > > > > 
>> > > > > 
>> > > > > > Suggested-by: Jiri Pirko <jiri@mellanox.com>
>> > > > > > 
>> > > > > 
>> > > > > Will add.
>> > > > > 
>> > > > > > 
>
>> > > I believe that this struct should go into:
>> > > include/uapi/linux/netlink.h
>> > > 
>> > > struct nla_flag_bits {
>> > > 	__u32 nla_flag_bits_values;
>> > > 	__u32 nla_flag_bits_selector;
>> > > };
>> > > 
>> > > Then you can use it from userspace and everywhere in kernel.
>> > > 
>> > That file seems to be very out of place for this stuff.
>> 
>> Howcome? It is a common netlink api.
>> 
>
>Take a look at that file's content. It talks about what goes in
>the netlink header. Adding types in it seems out of place.

Bit less than rtnetlink.h. But I agree it is not optimal. Re optimal,
please see below.


>
>
>> > NLA_XXX are kernel side types.  They are part of net/netlink.h which is
>> > not uapi accessible.
>> > David Ahern has submitted a patch to move all those defines to iproute2.
>> > Will make sense to move these to a uapi/linux/netlink-types.h but that
>> > is waay beyond the scope of this patch set.
>> 
>> Well, I don't think so :)
>> 
>> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS
>> enum value. They should be in the same uapi file. That makes sense to me.
>> 
>
>Sure - they should be in the same file. But is it uapi/linux/netlink.h?

Might be the netlink-types.h you mentioned above.

ccing DavidA.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
  2017-06-12 13:57         ` Jamal Hadi Salim
@ 2017-06-12 14:16           ` Jiri Pirko
  0 siblings, 0 replies; 28+ messages in thread
From: Jiri Pirko @ 2017-06-12 14:16 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

Mon, Jun 12, 2017 at 03:57:05PM CEST, jhs@mojatatu.com wrote:
>On 17-06-12 07:47 AM, Jiri Pirko wrote:
>> Mon, Jun 12, 2017 at 01:16:05PM CEST, jhs@mojatatu.com wrote:
>> > On 17-06-11 10:13 AM, Jiri Pirko wrote:
>
>> > This also is not useful.
>> > It happens to be ok for this use case but not for the
>> > general case. i.e.
>> > We need to get the whole struct not just the values
>> > and use the selector to pick what bits are affected.
>> > Example if bit X is set to 1 in selector and bit X in value
>> > is 0, then we set the kernel's bit X to 0.
>> 
>> Sure, have another helper for selector then.
>> Or, you can have:
>> 
>> 	struct nla_flag_bits *fb;
>> 	fb = nla_get_flag_bits(tb[TCA_ROOT_FLAGS]);
>> 
>> Or all 3 helpers. My point is, it is a specific netlink attribute with
>> specific format, it should have get/put helpers.
>> 
>
>I did try nla_get_flag_bits(tb[TCA_ROOT_FLAGS], &fb) so i dont have
>to do alloc/free - it ended being a single line function which
>does a memcpy.
>Is this really necessary?

Yes please, it is necessary! By the same logic, nla_get_u32 would not be
necessary.

I don't understand why you need alloc/free...


>The user of this structure better know it to make use of it.
>
>cheers,
>jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 14:14                 ` Jiri Pirko
@ 2017-06-12 15:00                   ` David Ahern
  2017-06-12 19:22                     ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: David Ahern @ 2017-06-12 15:00 UTC (permalink / raw)
  To: Jiri Pirko, Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 6/12/17 8:14 AM, Jiri Pirko wrote:
>>> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS
>>> enum value. They should be in the same uapi file. That makes sense to me.
>>>
>>
>> Sure - they should be in the same file. But is it uapi/linux/netlink.h?
> 
> Might be the netlink-types.h you mentioned above.
> 
> ccing DavidA.
> 

Just saw this patch set this morning. Few comments:

1. I think nla_bitfield or nla_bitmap is a better name than nla_flag_bits

2. The length should be open ended with the size of the array determined
by nla_len / sizeof(struct nla_bitfield). That allows this to be
extended to an arbitrary large bitfield as needed.

3. IMO since these are nla prefixes and new NLA type they should be in
uapi/linux/netlink.h

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 15:00                   ` David Ahern
@ 2017-06-12 19:22                     ` Jiri Pirko
  2017-06-12 19:58                       ` David Ahern
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-12 19:22 UTC (permalink / raw)
  To: David Ahern
  Cc: Jamal Hadi Salim, davem, netdev, xiyou.wangcong, eric.dumazet,
	simon.horman, mrv

Mon, Jun 12, 2017 at 05:00:41PM CEST, dsahern@gmail.com wrote:
>On 6/12/17 8:14 AM, Jiri Pirko wrote:
>>>> The thing is, struct nla_flag_bits is tightly coupled with NLA_FLAG_BITS
>>>> enum value. They should be in the same uapi file. That makes sense to me.
>>>>
>>>
>>> Sure - they should be in the same file. But is it uapi/linux/netlink.h?
>> 
>> Might be the netlink-types.h you mentioned above.
>> 
>> ccing DavidA.
>> 
>
>Just saw this patch set this morning. Few comments:
>
>1. I think nla_bitfield or nla_bitmap is a better name than nla_flag_bits

ack


>
>2. The length should be open ended with the size of the array determined
>by nla_len / sizeof(struct nla_bitfield). That allows this to be
>extended to an arbitrary large bitfield as needed.

Yeah, I was thinking about that as well. Seems handy to have this
generic len.


>
>3. IMO since these are nla prefixes and new NLA type they should be in
>uapi/linux/netlink.h

Including NLA_* type enum? I think it is reasonable.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 19:22                     ` Jiri Pirko
@ 2017-06-12 19:58                       ` David Ahern
  2017-06-13  5:32                         ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: David Ahern @ 2017-06-12 19:58 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Jamal Hadi Salim, davem, netdev, xiyou.wangcong, eric.dumazet,
	simon.horman, mrv

On 6/12/17 1:22 PM, Jiri Pirko wrote:
> 
>> 3. IMO since these are nla prefixes and new NLA type they should be in
>> uapi/linux/netlink.h
> Including NLA_* type enum? I think it is reasonable.

well, maybe not the NLA_BITFIELD. That enum is for policy validation
kernel side so not really part of the API.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-12 19:58                       ` David Ahern
@ 2017-06-13  5:32                         ` Jiri Pirko
  2017-06-13 10:53                           ` Jamal Hadi Salim
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-13  5:32 UTC (permalink / raw)
  To: David Ahern
  Cc: Jamal Hadi Salim, davem, netdev, xiyou.wangcong, eric.dumazet,
	simon.horman, mrv

Mon, Jun 12, 2017 at 09:58:33PM CEST, dsahern@gmail.com wrote:
>On 6/12/17 1:22 PM, Jiri Pirko wrote:
>> 
>>> 3. IMO since these are nla prefixes and new NLA type they should be in
>>> uapi/linux/netlink.h
>> Including NLA_* type enum? I think it is reasonable.
>
>well, maybe not the NLA_BITFIELD. That enum is for policy validation
>kernel side so not really part of the API.

Yeah, now I see it. Agreed.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-13  5:32                         ` Jiri Pirko
@ 2017-06-13 10:53                           ` Jamal Hadi Salim
  2017-06-13 11:25                             ` Jiri Pirko
  0 siblings, 1 reply; 28+ messages in thread
From: Jamal Hadi Salim @ 2017-06-13 10:53 UTC (permalink / raw)
  To: Jiri Pirko, David Ahern
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 17-06-13 01:32 AM, Jiri Pirko wrote:
> Mon, Jun 12, 2017 at 09:58:33PM CEST, dsahern@gmail.com wrote:
>> On 6/12/17 1:22 PM, Jiri Pirko wrote:
>>>
>>>> 3. IMO since these are nla prefixes and new NLA type they should be in
>>>> uapi/linux/netlink.h
>>> Including NLA_* type enum? I think it is reasonable.
>>
>> well, maybe not the NLA_BITFIELD. That enum is for policy validation
>> kernel side so not really part of the API.
> 
> Yeah, now I see it. Agreed.
> 

Jiri, you agreed to a name change? ;-> I want to have some of what
David A. ate yesterday.

I agree it is a good idea to have arbitrary size bitmask so we dont
run out of bit space but we need to restrict the max length possible.
I dont agree to using the netlink.h as the best location for this
but lets move on.
Do you or David A. want to take a crack at this?  I am a little tied
up.

cheers,
jamal

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-13 10:53                           ` Jamal Hadi Salim
@ 2017-06-13 11:25                             ` Jiri Pirko
  2017-06-13 13:58                               ` David Ahern
  0 siblings, 1 reply; 28+ messages in thread
From: Jiri Pirko @ 2017-06-13 11:25 UTC (permalink / raw)
  To: Jamal Hadi Salim
  Cc: David Ahern, davem, netdev, xiyou.wangcong, eric.dumazet,
	simon.horman, mrv

Tue, Jun 13, 2017 at 12:53:00PM CEST, jhs@mojatatu.com wrote:
>On 17-06-13 01:32 AM, Jiri Pirko wrote:
>> Mon, Jun 12, 2017 at 09:58:33PM CEST, dsahern@gmail.com wrote:
>> > On 6/12/17 1:22 PM, Jiri Pirko wrote:
>> > > 
>> > > > 3. IMO since these are nla prefixes and new NLA type they should be in
>> > > > uapi/linux/netlink.h
>> > > Including NLA_* type enum? I think it is reasonable.
>> > 
>> > well, maybe not the NLA_BITFIELD. That enum is for policy validation
>> > kernel side so not really part of the API.
>> 
>> Yeah, now I see it. Agreed.
>> 
>
>Jiri, you agreed to a name change? ;-> I want to have some of what
>David A. ate yesterday.

I ok with either of the tree (1 Jamal's, 2 DaveA's) variants.


>
>I agree it is a good idea to have arbitrary size bitmask so we dont
>run out of bit space but we need to restrict the max length possible.
>I dont agree to using the netlink.h as the best location for this
>but lets move on.

Suggest a better place, I don't see one.


>Do you or David A. want to take a crack at this?  I am a little tied
>up.

No time right now. Maybe in July I could allocate some cycles for this.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS
  2017-06-13 11:25                             ` Jiri Pirko
@ 2017-06-13 13:58                               ` David Ahern
  0 siblings, 0 replies; 28+ messages in thread
From: David Ahern @ 2017-06-13 13:58 UTC (permalink / raw)
  To: Jiri Pirko, Jamal Hadi Salim
  Cc: davem, netdev, xiyou.wangcong, eric.dumazet, simon.horman, mrv

On 6/13/17 5:25 AM, Jiri Pirko wrote:
> 
>> Do you or David A. want to take a crack at this?  I am a little tied
>> up.
> No time right now. Maybe in July I could allocate some cycles for this.

I'll see what I can do this weekend.

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2017-06-13 13:59 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-11 11:53 [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim
2017-06-11 11:53 ` [PATCH net-next v10 1/4] net netlink: Add new type NLA_FLAG_BITS Jamal Hadi Salim
2017-06-11 13:49   ` Jiri Pirko
2017-06-11 17:38     ` Jamal Hadi Salim
2017-06-11 18:37       ` Jamal Hadi Salim
2017-06-12 10:34         ` Jiri Pirko
2017-06-12 11:10           ` Jamal Hadi Salim
2017-06-12 11:43             ` Jiri Pirko
2017-06-12 13:51               ` Jamal Hadi Salim
2017-06-12 14:14                 ` Jiri Pirko
2017-06-12 15:00                   ` David Ahern
2017-06-12 19:22                     ` Jiri Pirko
2017-06-12 19:58                       ` David Ahern
2017-06-13  5:32                         ` Jiri Pirko
2017-06-13 10:53                           ` Jamal Hadi Salim
2017-06-13 11:25                             ` Jiri Pirko
2017-06-13 13:58                               ` David Ahern
2017-06-11 11:53 ` [PATCH net-next v10 2/4] net sched actions: Use proper root attribute table for actions Jamal Hadi Salim
2017-06-11 13:55   ` Jiri Pirko
2017-06-11 11:53 ` [PATCH net-next v10 3/4] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch Jamal Hadi Salim
2017-06-11 14:13   ` Jiri Pirko
2017-06-11 17:45     ` Jamal Hadi Salim
2017-06-12 11:16     ` Jamal Hadi Salim
2017-06-12 11:47       ` Jiri Pirko
2017-06-12 13:57         ` Jamal Hadi Salim
2017-06-12 14:16           ` Jiri Pirko
2017-06-11 11:53 ` [PATCH net-next v10 4/4] net sched actions: add time filter for action dumping Jamal Hadi Salim
2017-06-11 12:06 ` [PATCH net-next v10 0/4] net sched actions: improve dump performance Jamal Hadi Salim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.