[PATCH net-next 0/7] bridge: per-vlan stats

[PATCH net-next 0/7] bridge: per-vlan stats

[Nikolay Aleksandrov]

Hi,
This set adds support for bridge per-vlan statistics.
In order to be able to dump statistics we need a way to continue
dumping after reaching maximum size, thus patches 01-03 extend the new
stats API with a per-device extended link stats attribute and callback
which can save its local state and continue where it left off afterwards.
I considered using the already existing "fill_xstats" callback but it gets
confusing since we need to separate the linkinfo dump from the new stats
api dump and adding a flag/argument to do that just looks messy. I don't
think the rtnl_link_ops size is an issue, so adding these seemed like the
cleaner approach.

Patch 05 converts the pvid to a pointer so we can consolidate the vlan
stats accounting paths later, also allows to simplify the pvid code.
Patches 06 and 07 add the stats support and netlink dump support
respectively.
I've tested this set with both old and modified iproute2, kmemleak on and
some traffic stress tests while adding/removing vlans and ports.

Thank you,
 Nik

Note: Jamal I haven't forgotten about the per-port per-vlan stats, I've got
a follow-up patch that adds it. You can easily see that the infrastructure
for private port/vlan stats is in place after this set. Though the stats
api will need some more changes to support that.


Nikolay Aleksandrov (7):
  net: rtnetlink: allow rtnl_fill_statsinfo to save private state
    counter
  net: rtnetlink: allow only one idx saving stats attribute
  net: rtnetlink: add linkxstats callbacks and attribute
  net: constify is_skb_forwardable's arguments
  bridge: vlan: RCUify pvid
  bridge: vlan: learn to count
  bridge: netlink: export per-vlan stats

 include/linux/netdevice.h      |   3 +-
 include/net/rtnetlink.h        |  10 +++
 include/uapi/linux/if_bridge.h |   8 +++
 include/uapi/linux/if_link.h   |   9 +++
 net/bridge/br_netlink.c        |  80 ++++++++++++++++++++----
 net/bridge/br_private.h        |  32 +++++-----
 net/bridge/br_vlan.c           | 134 +++++++++++++++++++++++++++++------------
 net/core/dev.c                 |   2 +-
 net/core/rtnetlink.c           |  64 +++++++++++++++++---
 9 files changed, 266 insertions(+), 76 deletions(-)

-- 
2.4.11

[PATCH net-next 1/7] net: rtnetlink: allow rtnl_fill_statsinfo to save private state counter

[Nikolay Aleksandrov]

The new lidx argument allows the current dumping device to save a
private state counter which would enable it to continue dumping from
where it left off.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/core/rtnetlink.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 5ec059d52823..aeb2fa9b1cda 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3446,11 +3446,13 @@ out:
 
 static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 			       int type, u32 pid, u32 seq, u32 change,
-			       unsigned int flags, unsigned int filter_mask)
+			       unsigned int flags, unsigned int filter_mask,
+			       int *lidx)
 {
 	struct if_stats_msg *ifsm;
 	struct nlmsghdr *nlh;
 	struct nlattr *attr;
+	int s_lidx = *lidx;
 
 	ASSERT_RTNL();
 
@@ -3480,7 +3482,11 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 	return 0;
 
 nla_put_failure:
-	nlmsg_cancel(skb, nlh);
+	/* If we haven't made progress, it's a real error */
+	if (s_lidx == *lidx)
+		nlmsg_cancel(skb, nlh);
+	else
+		nlmsg_end(skb, nlh);
 
 	return -EMSGSIZE;
 }
@@ -3507,6 +3513,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct net_device *dev = NULL;
 	struct sk_buff *nskb;
 	u32 filter_mask;
+	int lidx = 0;
 	int err;
 
 	ifsm = nlmsg_data(nlh);
@@ -3528,7 +3535,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
 
 	err = rtnl_fill_statsinfo(nskb, dev, RTM_NEWSTATS,
 				  NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0,
-				  0, filter_mask);
+				  0, filter_mask, &lidx);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_stats_size */
 		WARN_ON(err == -EMSGSIZE);
@@ -3545,7 +3552,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	struct net *net = sock_net(skb->sk);
 	struct if_stats_msg *ifsm;
 	int h, s_h;
-	int idx = 0, s_idx;
+	int idx = 0, s_idx, s_lidx;
 	struct net_device *dev;
 	struct hlist_head *head;
 	unsigned int flags = NLM_F_MULTI;
@@ -3554,6 +3561,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 	s_h = cb->args[0];
 	s_idx = cb->args[1];
+	s_lidx = cb->args[2];
 
 	cb->seq = net->dev_base_seq;
 
@@ -3571,7 +3579,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 			err = rtnl_fill_statsinfo(skb, dev, RTM_NEWSTATS,
 						  NETLINK_CB(cb->skb).portid,
 						  cb->nlh->nlmsg_seq, 0,
-						  flags, filter_mask);
+						  flags, filter_mask, &s_lidx);
 			/* If we ran out of room on the first message,
 			 * we're in trouble
 			 */
@@ -3579,13 +3587,14 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 
 			if (err < 0)
 				goto out;
-
+			s_lidx = 0;
 			nl_dump_check_consistent(cb, nlmsg_hdr(skb));
 cont:
 			idx++;
 		}
 	}
 out:
+	cb->args[2] = s_lidx;
 	cb->args[1] = idx;
 	cb->args[0] = h;
 
-- 
2.4.11

[PATCH net-next 2/7] net: rtnetlink: allow only one idx saving stats attribute

[Nikolay Aleksandrov]

We can't allow more than one stats attribute which uses the local idx
since the result will be a mess. This is a simple check to make sure
only one is being used at a time. Later when the filter_mask's 32 bits
are over we can switch to a bitmap.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 include/net/rtnetlink.h |  6 ++++++
 net/core/rtnetlink.c    | 17 +++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 2f87c1ba13de..3f3b0b1b8722 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -150,4 +150,10 @@ int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len);
 
 #define MODULE_ALIAS_RTNL_LINK(kind) MODULE_ALIAS("rtnl-link-" kind)
 
+/* at most one attribute which can save a local idx is allowed to be set
+ * IFLA_STATS_IDX_ATTR_MASK has all the idx saving attributes set and is
+ * used to check if more than one is being requested
+ */
+#define IFLA_STATS_IDX_ATTR_MASK 0
+
 #endif
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index aeb2fa9b1cda..ea03b6cd3d3c 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3512,7 +3512,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
 	struct if_stats_msg *ifsm;
 	struct net_device *dev = NULL;
 	struct sk_buff *nskb;
-	u32 filter_mask;
+	u32 filter_mask, lidx_filter;
 	int lidx = 0;
 	int err;
 
@@ -3529,6 +3529,14 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
 	if (!filter_mask)
 		return -EINVAL;
 
+	/* only one attribute which can save a local idx is allowed at a time
+	 * even though rtnl_stats_get doesn't save the lidx, we need to be
+	 * consistent with the dump side and error out
+	 */
+	lidx_filter = filter_mask & IFLA_STATS_IDX_ATTR_MASK;
+	if (lidx_filter && !is_power_of_2(lidx_filter))
+		return -EINVAL;
+
 	nskb = nlmsg_new(if_nlmsg_stats_size(dev, filter_mask), GFP_KERNEL);
 	if (!nskb)
 		return -ENOBUFS;
@@ -3556,7 +3564,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	struct net_device *dev;
 	struct hlist_head *head;
 	unsigned int flags = NLM_F_MULTI;
-	u32 filter_mask = 0;
+	u32 filter_mask = 0, lidx_filter;
 	int err;
 
 	s_h = cb->args[0];
@@ -3570,6 +3578,11 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	if (!filter_mask)
 		return -EINVAL;
 
+	/* only one attribute which can save a local idx is allowed at a time */
+	lidx_filter = filter_mask & IFLA_STATS_IDX_ATTR_MASK;
+	if (lidx_filter && !is_power_of_2(lidx_filter))
+		return -EINVAL;
+
 	for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
 		idx = 0;
 		head = &net->dev_index_head[h];
-- 
2.4.11

Re: [PATCH net-next 2/7] net: rtnetlink: allow only one idx saving stats attribute

[Roopa Prabhu]

On 4/27/16, 9:18 AM, Nikolay Aleksandrov wrote:
> We can't allow more than one stats attribute which uses the local idx
> since the result will be a mess. This is a simple check to make sure
> only one is being used at a time. Later when the filter_mask's 32 bits
> are over we can switch to a bitmap.
>
> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
> ---
>  include/net/rtnetlink.h |  6 ++++++
>  net/core/rtnetlink.c    | 17 +++++++++++++++--
>  2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
> index 2f87c1ba13de..3f3b0b1b8722 100644
> --- a/include/net/rtnetlink.h
> +++ b/include/net/rtnetlink.h
> @@ -150,4 +150,10 @@ int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len);
>  
>  #define MODULE_ALIAS_RTNL_LINK(kind) MODULE_ALIAS("rtnl-link-" kind)
>  
> +/* at most one attribute which can save a local idx is allowed to be set
> + * IFLA_STATS_IDX_ATTR_MASK has all the idx saving attributes set and is
> + * used to check if more than one is being requested
> + */
> +#define IFLA_STATS_IDX_ATTR_MASK 0
> +
>  #endif
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index aeb2fa9b1cda..ea03b6cd3d3c 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -3512,7 +3512,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
>  	struct if_stats_msg *ifsm;
>  	struct net_device *dev = NULL;
>  	struct sk_buff *nskb;
> -	u32 filter_mask;
> +	u32 filter_mask, lidx_filter;
>  	int lidx = 0;
>  	int err;
>  
> @@ -3529,6 +3529,14 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh)
>  	if (!filter_mask)
>  		return -EINVAL;
>  
> +	/* only one attribute which can save a local idx is allowed at a time
> +	 * even though rtnl_stats_get doesn't save the lidx, we need to be
> +	 * consistent with the dump side and error out
> +	 */
> +	lidx_filter = filter_mask & IFLA_STATS_IDX_ATTR_MASK;
> +	if (lidx_filter && !is_power_of_2(lidx_filter))
> +		return -EINVAL;
> +
>  	nskb = nlmsg_new(if_nlmsg_stats_size(dev, filter_mask), GFP_KERNEL);
>  	if (!nskb)
>  		return -ENOBUFS;
> @@ -3556,7 +3564,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	struct net_device *dev;
>  	struct hlist_head *head;
>  	unsigned int flags = NLM_F_MULTI;
> -	u32 filter_mask = 0;
> +	u32 filter_mask = 0, lidx_filter;
>  	int err;
>  
>  	s_h = cb->args[0];
> @@ -3570,6 +3578,11 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	if (!filter_mask)
>  		return -EINVAL;
>  
> +	/* only one attribute which can save a local idx is allowed at a time */
> +	lidx_filter = filter_mask & IFLA_STATS_IDX_ATTR_MASK;
> +	if (lidx_filter && !is_power_of_2(lidx_filter))
> +		return -EINVAL;
> +
>  	
instead of introducing the restriction at this level, is it possible to use two args for this
like below and avoid the restriction ?
cb->args[2] = current filter being processed
cb->args[3] = private filter idx (your lidx)

Re: [PATCH net-next 2/7] net: rtnetlink: allow only one idx saving stats attribute

[Nikolay Aleksandrov]

On 04/28/2016 07:18 AM, Roopa Prabhu wrote:
> On 4/27/16, 9:18 AM, Nikolay Aleksandrov wrote:
>> We can't allow more than one stats attribute which uses the local idx
>> since the result will be a mess. This is a simple check to make sure
>> only one is being used at a time. Later when the filter_mask's 32 bits
>> are over we can switch to a bitmap.
>>
>> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> ---
>>  include/net/rtnetlink.h |  6 ++++++
>>  net/core/rtnetlink.c    | 17 +++++++++++++++--
>>  2 files changed, 21 insertions(+), 2 deletions(-)
>>
[snip]
>> @@ -3570,6 +3578,11 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>  	if (!filter_mask)
>>  		return -EINVAL;
>>  
>> +	/* only one attribute which can save a local idx is allowed at a time */
>> +	lidx_filter = filter_mask & IFLA_STATS_IDX_ATTR_MASK;
>> +	if (lidx_filter && !is_power_of_2(lidx_filter))
>> +		return -EINVAL;
>> +
>>  	
> instead of introducing the restriction at this level, is it possible to use two args for this
> like below and avoid the restriction ?
> cb->args[2] = current filter being processed
> cb->args[3] = private filter idx (your lidx)
> 

So to allow having any number of idx saving callbacks ? I think this will introduce more complexity
as we'll have to differentiate between 3 types of filters now - non-idx saving, idx saving but passed
and current idx saving attribute. We will dump the non-idx saving on each call, but then skip some
and continue dumping..
I'll give it a try to see how it looks. :-)

Thanks,
 Nik

[PATCH net-next 3/7] net: rtnetlink: add linkxstats callbacks and attribute

[Nikolay Aleksandrov]

Add callbacks to calculate the size and fill link extended statistics
which can be split into multiple messages and are dumped via the new
rtnl stats API (RTM_GETSTATS) with the IFLA_STATS_LINK_XSTATS attribute.
Also add that attribute to the idx mask check since it is expected to
be able to save state and resume dumping (e.g. future bridge per-vlan
stats will be dumped via this attribute and callbacks).

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 include/net/rtnetlink.h      |  6 +++++-
 include/uapi/linux/if_link.h |  8 ++++++++
 net/core/rtnetlink.c         | 26 ++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/include/net/rtnetlink.h b/include/net/rtnetlink.h
index 3f3b0b1b8722..b449c1f3416f 100644
--- a/include/net/rtnetlink.h
+++ b/include/net/rtnetlink.h
@@ -95,6 +95,10 @@ struct rtnl_link_ops {
 						   const struct net_device *dev,
 						   const struct net_device *slave_dev);
 	struct net		*(*get_link_net)(const struct net_device *dev);
+	size_t			(*get_linkxstats_size)(const struct net_device *dev);
+	int			(*fill_linkxstats)(struct sk_buff *skb,
+						   const struct net_device *dev,
+						   int *lidx);
 };
 
 int __rtnl_link_register(struct rtnl_link_ops *ops);
@@ -154,6 +158,6 @@ int rtnl_nla_parse_ifla(struct nlattr **tb, const struct nlattr *head, int len);
  * IFLA_STATS_IDX_ATTR_MASK has all the idx saving attributes set and is
  * used to check if more than one is being requested
  */
-#define IFLA_STATS_IDX_ATTR_MASK 0
+#define IFLA_STATS_IDX_ATTR_MASK IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_XSTATS)
 
 #endif
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index ba69d4447249..1b874e26b15b 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -798,6 +798,7 @@ struct if_stats_msg {
 enum {
 	IFLA_STATS_UNSPEC, /* also used as 64bit pad attribute */
 	IFLA_STATS_LINK_64,
+	IFLA_STATS_LINK_XSTATS,
 	__IFLA_STATS_MAX,
 };
 
@@ -805,4 +806,11 @@ enum {
 
 #define IFLA_STATS_FILTER_BIT(ATTR)	(1 << (ATTR - 1))
 
+/* These are embedded into IFLA_STATS_LINK_XSTATS */
+enum {
+	LINK_XSTATS_UNSPEC,
+	__LINK_XSTATS_MAX
+};
+#define LINK_XSTATS_MAX (__LINK_XSTATS_MAX - 1)
+
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ea03b6cd3d3c..9637618c408d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3477,6 +3477,23 @@ static int rtnl_fill_statsinfo(struct sk_buff *skb, struct net_device *dev,
 		dev_get_stats(dev, sp);
 	}
 
+	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_XSTATS)) {
+		const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
+
+		if (ops && ops->fill_linkxstats) {
+			int err;
+
+			attr = nla_nest_start(skb, IFLA_STATS_LINK_XSTATS);
+			if (!attr)
+				goto nla_put_failure;
+
+			err = ops->fill_linkxstats(skb, dev, lidx);
+			nla_nest_end(skb, attr);
+			if (err)
+				goto nla_put_failure;
+		}
+	}
+
 	nlmsg_end(skb, nlh);
 
 	return 0;
@@ -3503,6 +3520,15 @@ static size_t if_nlmsg_stats_size(const struct net_device *dev,
 	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_64))
 		size += nla_total_size_64bit(sizeof(struct rtnl_link_stats64));
 
+	if (filter_mask & IFLA_STATS_FILTER_BIT(IFLA_STATS_LINK_XSTATS)) {
+		const struct rtnl_link_ops *ops = dev->rtnl_link_ops;
+
+		if (ops && ops->get_linkxstats_size)
+			size += nla_total_size(ops->get_linkxstats_size(dev));
+		/* anything dumped is embedded in IFLA_STATS_LINK_XSTATS */
+		size += nla_total_size(0);
+	}
+
 	return size;
 }
 
-- 
2.4.11

[PATCH net-next 4/7] net: constify is_skb_forwardable's arguments

[Nikolay Aleksandrov]

is_skb_forwardable is not supposed to change anything so constify its
arguments

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 include/linux/netdevice.h | 3 ++-
 net/core/dev.c            | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1f6d5db471a2..85d56e7cd46b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3263,7 +3263,8 @@ struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
 				    struct netdev_queue *txq, int *ret);
 int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
 int dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
-bool is_skb_forwardable(struct net_device *dev, struct sk_buff *skb);
+bool is_skb_forwardable(const struct net_device *dev,
+			const struct sk_buff *skb);
 
 extern int		netdev_budget;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index 6324bc9267f7..36f0170a0754 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1741,7 +1741,7 @@ static inline void net_timestamp_set(struct sk_buff *skb)
 			__net_timestamp(SKB);		\
 	}						\
 
-bool is_skb_forwardable(struct net_device *dev, struct sk_buff *skb)
+bool is_skb_forwardable(const struct net_device *dev, const struct sk_buff *skb)
 {
 	unsigned int len;
 
-- 
2.4.11

[PATCH net-next 5/7] bridge: vlan: RCUify pvid

[Nikolay Aleksandrov]

Make pvid a pointer to a vlan struct and RCUify the access to it. Vlans
are already RCU-protected so the pvid vlan entry cannot disappear
without being initialized to NULL and going through a grace period first.
This change is necessary for the upcoming vlan counters and also would
serve to later move to vlan passing via a pointer instead of id.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 net/bridge/br_netlink.c | 23 ++++++++++-----------
 net/bridge/br_private.h | 16 +--------------
 net/bridge/br_vlan.c    | 54 +++++++++++++++++++++++--------------------------
 3 files changed, 37 insertions(+), 56 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e9c635eae24d..f33d95b0f5d3 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -24,22 +24,22 @@
 static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg,
 				u32 filter_mask)
 {
-	struct net_bridge_vlan *v;
+	struct net_bridge_vlan *v, *pvid;
 	u16 vid_range_start = 0, vid_range_end = 0, vid_range_flags = 0;
-	u16 flags, pvid;
 	int num_vlans = 0;
+	u16 flags;
 
 	if (!(filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED))
 		return 0;
 
-	pvid = br_get_pvid(vg);
+	pvid = rcu_dereference(vg->pvid);
 	/* Count number of vlan infos */
 	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		flags = 0;
 		/* only a context, bridge vlan not activated */
 		if (!br_vlan_should_use(v))
 			continue;
-		if (v->vid == pvid)
+		if (v == pvid)
 			flags |= BRIDGE_VLAN_INFO_PVID;
 
 		if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
@@ -243,21 +243,21 @@ nla_put_failure:
 static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
 					 struct net_bridge_vlan_group *vg)
 {
-	struct net_bridge_vlan *v;
+	struct net_bridge_vlan *v, *pvid;
 	u16 vid_range_start = 0, vid_range_end = 0, vid_range_flags = 0;
-	u16 flags, pvid;
 	int err = 0;
+	u16 flags;
 
 	/* Pack IFLA_BRIDGE_VLAN_INFO's for every vlan
 	 * and mark vlan info with begin and end flags
 	 * if vlaninfo represents a range
 	 */
-	pvid = br_get_pvid(vg);
+	pvid = rcu_dereference(vg->pvid);
 	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		flags = 0;
 		if (!br_vlan_should_use(v))
 			continue;
-		if (v->vid == pvid)
+		if (v == pvid)
 			flags |= BRIDGE_VLAN_INFO_PVID;
 
 		if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
@@ -298,18 +298,17 @@ initvars:
 static int br_fill_ifvlaninfo(struct sk_buff *skb,
 			      struct net_bridge_vlan_group *vg)
 {
+	struct net_bridge_vlan *v, *pvid;
 	struct bridge_vlan_info vinfo;
-	struct net_bridge_vlan *v;
-	u16 pvid;
 
-	pvid = br_get_pvid(vg);
+	pvid = rcu_dereference(vg->pvid);
 	list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
 		if (!br_vlan_should_use(v))
 			continue;
 
 		vinfo.vid = v->vid;
 		vinfo.flags = 0;
-		if (v->vid == pvid)
+		if (v == pvid)
 			vinfo.flags |= BRIDGE_VLAN_INFO_PVID;
 
 		if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1b5d145dfcbf..50d70b5eb307 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -130,8 +130,8 @@ struct net_bridge_vlan {
 struct net_bridge_vlan_group {
 	struct rhashtable		vlan_hash;
 	struct list_head		vlan_list;
+	struct net_bridge_vlan __rcu	*pvid;
 	u16				num_vlans;
-	u16				pvid;
 };
 
 struct net_bridge_fdb_entry
@@ -741,15 +741,6 @@ static inline int br_vlan_get_tag(const struct sk_buff *skb, u16 *vid)
 	return err;
 }
 
-static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
-{
-	if (!vg)
-		return 0;
-
-	smp_rmb();
-	return vg->pvid;
-}
-
 static inline int br_vlan_enabled(struct net_bridge *br)
 {
 	return br->vlan_enabled;
@@ -835,11 +826,6 @@ static inline u16 br_vlan_get_tag(const struct sk_buff *skb, u16 *tag)
 	return 0;
 }
 
-static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
-{
-	return 0;
-}
-
 static inline int br_vlan_enabled(struct net_bridge *br)
 {
 	return 0;
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index e001152d6ad1..4fab7665df8c 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -31,22 +31,11 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
 	return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
 }
 
-static void __vlan_add_pvid(struct net_bridge_vlan_group *vg, u16 vid)
+/* __vlan_delete_pvid is just __vlan_set_pvid(vg, NULL) */
+static void __vlan_set_pvid(struct net_bridge_vlan_group *vg,
+			    struct net_bridge_vlan *v)
 {
-	if (vg->pvid == vid)
-		return;
-
-	smp_wmb();
-	vg->pvid = vid;
-}
-
-static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
-{
-	if (vg->pvid != vid)
-		return;
-
-	smp_wmb();
-	vg->pvid = 0;
+	rcu_assign_pointer(vg->pvid, v);
 }
 
 static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
@@ -59,9 +48,9 @@ static void __vlan_add_flags(struct net_bridge_vlan *v, u16 flags)
 		vg = nbp_vlan_group(v->port);
 
 	if (flags & BRIDGE_VLAN_INFO_PVID)
-		__vlan_add_pvid(vg, v->vid);
-	else
-		__vlan_delete_pvid(vg, v->vid);
+		__vlan_set_pvid(vg, v);
+	else if (rtnl_dereference(vg->pvid) == v)
+		__vlan_set_pvid(vg, NULL);
 
 	if (flags & BRIDGE_VLAN_INFO_UNTAGGED)
 		v->flags |= BRIDGE_VLAN_INFO_UNTAGGED;
@@ -285,7 +274,9 @@ static int __vlan_del(struct net_bridge_vlan *v)
 		masterv = v->brvlan;
 	}
 
-	__vlan_delete_pvid(vg, v->vid);
+	if (rtnl_dereference(vg->pvid) == v)
+		__vlan_set_pvid(vg, NULL);
+
 	if (p) {
 		err = __vlan_vid_del(p->dev, p->br, v->vid);
 		if (err)
@@ -320,7 +311,7 @@ static void __vlan_flush(struct net_bridge_vlan_group *vg)
 {
 	struct net_bridge_vlan *vlan, *tmp;
 
-	__vlan_delete_pvid(vg, vg->pvid);
+	__vlan_set_pvid(vg, NULL);
 	list_for_each_entry_safe(vlan, tmp, &vg->vlan_list, vlist)
 		__vlan_del(vlan);
 }
@@ -404,29 +395,29 @@ static bool __allowed_ingress(struct net_bridge_vlan_group *vg, __be16 proto,
 	}
 
 	if (!*vid) {
-		u16 pvid = br_get_pvid(vg);
+		v = rcu_dereference(vg->pvid);
 
 		/* Frame had a tag with VID 0 or did not have a tag.
 		 * See if pvid is set on this port.  That tells us which
 		 * vlan untagged or priority-tagged traffic belongs to.
 		 */
-		if (!pvid)
+		if (!v)
 			goto drop;
 
 		/* PVID is set on this port.  Any untagged or priority-tagged
 		 * ingress frame is considered to belong to this vlan.
 		 */
-		*vid = pvid;
+		*vid = v->vid;
 		if (likely(!tagged))
 			/* Untagged Frame. */
-			__vlan_hwaccel_put_tag(skb, proto, pvid);
+			__vlan_hwaccel_put_tag(skb, proto, v->vid);
 		else
 			/* Priority-tagged Frame.
 			 * At this point, We know that skb->vlan_tci had
 			 * VLAN_TAG_PRESENT bit and its VID field was 0x000.
 			 * We update only VID field and preserve PCP field.
 			 */
-			skb->vlan_tci |= pvid;
+			skb->vlan_tci |= v->vid;
 
 		return true;
 	}
@@ -451,6 +442,9 @@ bool br_allowed_ingress(const struct net_bridge *br,
 		BR_INPUT_SKB_CB(skb)->vlan_filtered = false;
 		return true;
 	}
+	/* if there's no vlan_group, there's nothing to match against */
+	if (!vg)
+		return false;
 
 	return __allowed_ingress(vg, br->vlan_proto, skb, vid);
 }
@@ -492,9 +486,11 @@ bool br_should_learn(struct net_bridge_port *p, struct sk_buff *skb, u16 *vid)
 		*vid = 0;
 
 	if (!*vid) {
-		*vid = br_get_pvid(vg);
-		if (!*vid)
+		struct net_bridge_vlan *v = rcu_dereference(vg->pvid);
+
+		if (!v)
 			return false;
+		*vid = v->vid;
 
 		return true;
 	}
@@ -713,9 +709,9 @@ int br_vlan_set_proto(struct net_bridge *br, unsigned long val)
 
 static bool vlan_default_pvid(struct net_bridge_vlan_group *vg, u16 vid)
 {
-	struct net_bridge_vlan *v;
+	struct net_bridge_vlan *v = rtnl_dereference(vg->pvid);
 
-	if (vid != vg->pvid)
+	if (v && vid != v->vid)
 		return false;
 
 	v = br_vlan_lookup(&vg->vlan_hash, vid);
-- 
2.4.11

[PATCH net-next 6/7] bridge: vlan: learn to count

[Nikolay Aleksandrov]

Add support for per-VLAN Tx/Rx statistics. Every global vlan context gets
allocated a per-cpu stats which is then set in each per-port vlan context
for quick access. The br_allowed_ingress() common function is used to
account for Rx packets and the br_handle_vlan() common function is used
to account for Tx packets.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
Note: maybe in the future it'd be better to rename br_allowed_ingress to
br_vlan_ingress() or something similar as it's not doing only checks.

 net/bridge/br_private.h | 11 +++++++++-
 net/bridge/br_vlan.c    | 53 +++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 50d70b5eb307..f6876ed718a5 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -77,12 +77,21 @@ struct bridge_mcast_querier {
 };
 #endif
 
+struct br_vlan_stats {
+	u64 rx_bytes;
+	u64 rx_packets;
+	u64 tx_bytes;
+	u64 tx_packets;
+	struct u64_stats_sync syncp;
+};
+
 /**
  * struct net_bridge_vlan - per-vlan entry
  *
  * @vnode: rhashtable member
  * @vid: VLAN id
  * @flags: bridge vlan flags
+ * @stats: per-cpu VLAN statistics
  * @br: if MASTER flag set, this points to a bridge struct
  * @port: if MASTER flag unset, this points to a port struct
  * @refcnt: if MASTER flag set, this is bumped for each port referencing it
@@ -100,6 +109,7 @@ struct net_bridge_vlan {
 	struct rhash_head		vnode;
 	u16				vid;
 	u16				flags;
+	struct br_vlan_stats __percpu	*stats;
 	union {
 		struct net_bridge	*br;
 		struct net_bridge_port	*port;
@@ -866,7 +876,6 @@ static inline struct net_bridge_vlan_group *nbp_vlan_group_rcu(
 {
 	return NULL;
 }
-
 #endif
 
 struct nf_br_ops {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 4fab7665df8c..d7a70c2ea3ec 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -151,6 +151,17 @@ static struct net_bridge_vlan *br_vlan_get_master(struct net_bridge *br, u16 vid
 	return masterv;
 }
 
+static void br_master_vlan_rcu_free(struct rcu_head *rcu)
+{
+	struct net_bridge_vlan *v;
+
+	v = container_of(rcu, struct net_bridge_vlan, rcu);
+	WARN_ON(!br_vlan_is_master(v));
+	free_percpu(v->stats);
+	v->stats = NULL;
+	kfree(v);
+}
+
 static void br_vlan_put_master(struct net_bridge_vlan *masterv)
 {
 	struct net_bridge_vlan_group *vg;
@@ -163,7 +174,7 @@ static void br_vlan_put_master(struct net_bridge_vlan *masterv)
 		rhashtable_remove_fast(&vg->vlan_hash,
 				       &masterv->vnode, br_vlan_rht_params);
 		__vlan_del_list(masterv);
-		kfree_rcu(masterv, rcu);
+		call_rcu(&masterv->rcu, br_master_vlan_rcu_free);
 	}
 }
 
@@ -219,6 +230,7 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags)
 		if (!masterv)
 			goto out_filt;
 		v->brvlan = masterv;
+		v->stats = masterv->stats;
 	}
 
 	/* Add the dev mac and count the vlan only if it's usable */
@@ -320,6 +332,7 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 			       struct net_bridge_vlan_group *vg,
 			       struct sk_buff *skb)
 {
+	struct br_vlan_stats *stats;
 	struct net_bridge_vlan *v;
 	u16 vid;
 
@@ -346,9 +359,14 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
 			return NULL;
 		}
 	}
+	stats = this_cpu_ptr(v->stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->tx_bytes += skb->len;
+	stats->tx_packets++;
+	u64_stats_update_end(&stats->syncp);
+
 	if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
 		skb->vlan_tci = 0;
-
 out:
 	return skb;
 }
@@ -357,7 +375,8 @@ out:
 static bool __allowed_ingress(struct net_bridge_vlan_group *vg, __be16 proto,
 			      struct sk_buff *skb, u16 *vid)
 {
-	const struct net_bridge_vlan *v;
+	struct br_vlan_stats *stats;
+	struct net_bridge_vlan *v;
 	bool tagged;
 
 	BR_INPUT_SKB_CB(skb)->vlan_filtered = true;
@@ -418,14 +437,21 @@ static bool __allowed_ingress(struct net_bridge_vlan_group *vg, __be16 proto,
 			 * We update only VID field and preserve PCP field.
 			 */
 			skb->vlan_tci |= v->vid;
-
-		return true;
+	} else {
+		/* Frame had a valid vlan tag.  See if vlan is allowed */
+		v = br_vlan_find(vg, *vid);
 	}
+	if (!v || !br_vlan_should_use(v))
+		goto drop;
+
+	stats = this_cpu_ptr(v->stats);
+	u64_stats_update_begin(&stats->syncp);
+	stats->rx_bytes += skb->len;
+	stats->rx_packets++;
+	u64_stats_update_end(&stats->syncp);
+
+	return true;
 
-	/* Frame had a valid vlan tag.  See if vlan is allowed */
-	v = br_vlan_find(vg, *vid);
-	if (v && br_vlan_should_use(v))
-		return true;
 drop:
 	kfree_skb(skb);
 	return false;
@@ -538,6 +564,11 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
 	if (!vlan)
 		return -ENOMEM;
 
+	vlan->stats = netdev_alloc_pcpu_stats(struct br_vlan_stats);
+	if (!vlan->stats) {
+		kfree(vlan);
+		return -ENOMEM;
+	}
 	vlan->vid = vid;
 	vlan->flags = flags | BRIDGE_VLAN_INFO_MASTER;
 	vlan->flags &= ~BRIDGE_VLAN_INFO_PVID;
@@ -545,8 +576,10 @@ int br_vlan_add(struct net_bridge *br, u16 vid, u16 flags)
 	if (flags & BRIDGE_VLAN_INFO_BRENTRY)
 		atomic_set(&vlan->refcnt, 1);
 	ret = __vlan_add(vlan, flags);
-	if (ret)
+	if (ret) {
+		free_percpu(vlan->stats);
 		kfree(vlan);
+	}
 
 	return ret;
 }
-- 
2.4.11

[PATCH net-next 7/7] bridge: netlink: export per-vlan stats

[Nikolay Aleksandrov]

Add a new LINK_XSTATS_BRIDGE_VLAN attribute and implement the
RTM_GETSTATS callbacks for IFLA_STATS_LINK_XSTATS (fill_linkxstats and
get_linkxstats_size) in order to export the per-vlan stats.

Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
---
 include/uapi/linux/if_bridge.h |  8 ++++++
 include/uapi/linux/if_link.h   |  1 +
 net/bridge/br_netlink.c        | 57 ++++++++++++++++++++++++++++++++++++++++++
 net/bridge/br_private.h        |  7 ++++++
 net/bridge/br_vlan.c           | 27 ++++++++++++++++++++
 5 files changed, 100 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 0536eefff9bf..3eb4e7145825 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -134,6 +134,14 @@ struct bridge_vlan_info {
 	__u16 vid;
 };
 
+struct bridge_vlan_xstats {
+	__u16 vid;
+	__u64 rx_bytes;
+	__u64 rx_packets;
+	__u64 tx_bytes;
+	__u64 tx_packets;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  *     [MDBA_MDB_ENTRY] = {
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 1b874e26b15b..7a9420a19720 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -809,6 +809,7 @@ enum {
 /* These are embedded into IFLA_STATS_LINK_XSTATS */
 enum {
 	LINK_XSTATS_UNSPEC,
+	LINK_XSTATS_BRIDGE_VLAN,
 	__LINK_XSTATS_MAX
 };
 #define LINK_XSTATS_MAX (__LINK_XSTATS_MAX - 1)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index f33d95b0f5d3..34b4fa6fd693 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1212,6 +1212,61 @@ static int br_fill_info(struct sk_buff *skb, const struct net_device *brdev)
 	return 0;
 }
 
+static size_t br_get_linkxstats_size(const struct net_device *dev)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	struct net_bridge_vlan_group *vg;
+	struct net_bridge_vlan *v;
+	int numvls = 0;
+
+	vg = br_vlan_group(br);
+	if (!vg || !vg->num_vlans)
+		return 0;
+
+	/* we need to count all, even placeholder entries */
+	list_for_each_entry(v, &vg->vlan_list, vlist)
+		numvls++;
+
+	return numvls * nla_total_size(sizeof(struct bridge_vlan_xstats));
+}
+
+static int br_fill_linkxstats(struct sk_buff *skb, const struct net_device *dev,
+			      int *lidx)
+{
+	struct net_bridge *br = netdev_priv(dev);
+	struct net_bridge_vlan *v, *pvid;
+	struct net_bridge_vlan_group *vg;
+	struct bridge_vlan_xstats vxi;
+	int vl_idx = 0;
+
+	vg = br_vlan_group(br);
+	if (!vg || !vg->num_vlans)
+		goto out;
+	pvid = rtnl_dereference(vg->pvid);
+	list_for_each_entry(v, &vg->vlan_list, vlist) {
+		struct br_vlan_stats stats;
+
+		if (vl_idx++ < *lidx)
+			continue;
+		memset(&vxi, 0, sizeof(vxi));
+		vxi.vid = v->vid;
+		br_vlan_get_stats(v, &stats);
+		vxi.rx_bytes = stats.rx_bytes;
+		vxi.rx_packets = stats.rx_packets;
+		vxi.tx_bytes = stats.tx_bytes;
+		vxi.tx_packets = stats.tx_packets;
+
+		if (nla_put(skb, LINK_XSTATS_BRIDGE_VLAN, sizeof(vxi), &vxi))
+			goto nla_put_failure;
+	}
+	*lidx = 0;
+out:
+	return 0;
+
+nla_put_failure:
+	*lidx = vl_idx;
+	return -EMSGSIZE;
+}
 
 static struct rtnl_af_ops br_af_ops __read_mostly = {
 	.family			= AF_BRIDGE,
@@ -1230,6 +1285,8 @@ struct rtnl_link_ops br_link_ops __read_mostly = {
 	.dellink		= br_dev_delete,
 	.get_size		= br_get_size,
 	.fill_info		= br_fill_info,
+	.fill_linkxstats	= br_fill_linkxstats,
+	.get_linkxstats_size	= br_get_linkxstats_size,
 
 	.slave_maxtype		= IFLA_BRPORT_MAX,
 	.slave_policy		= br_port_policy,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index f6876ed718a5..a10f7ed26f3b 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -709,6 +709,8 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
 void nbp_vlan_flush(struct net_bridge_port *port);
 int nbp_vlan_init(struct net_bridge_port *port);
 int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
+void br_vlan_get_stats(const struct net_bridge_vlan *v,
+		       struct br_vlan_stats *stats);
 
 static inline struct net_bridge_vlan_group *br_vlan_group(
 					const struct net_bridge *br)
@@ -876,6 +878,11 @@ static inline struct net_bridge_vlan_group *nbp_vlan_group_rcu(
 {
 	return NULL;
 }
+
+static inline void br_vlan_get_stats(const struct net_bridge_vlan *v,
+				     struct br_vlan_stats *stats)
+{
+}
 #endif
 
 struct nf_br_ops {
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index d7a70c2ea3ec..b39d9f5761d9 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -1029,3 +1029,30 @@ void nbp_vlan_flush(struct net_bridge_port *port)
 	synchronize_rcu();
 	__vlan_group_free(vg);
 }
+
+void br_vlan_get_stats(const struct net_bridge_vlan *v,
+		       struct br_vlan_stats *stats)
+{
+	int i;
+
+	memset(stats, 0, sizeof(*stats));
+	for_each_possible_cpu(i) {
+		u64 rxpackets, rxbytes, txpackets, txbytes;
+		struct br_vlan_stats *cpu_stats;
+		unsigned int start;
+
+		cpu_stats = per_cpu_ptr(v->stats, i);
+		do {
+			start = u64_stats_fetch_begin_irq(&cpu_stats->syncp);
+			rxpackets = cpu_stats->rx_packets;
+			rxbytes = cpu_stats->rx_bytes;
+			txbytes = cpu_stats->tx_bytes;
+			txpackets = cpu_stats->tx_packets;
+		} while (u64_stats_fetch_retry_irq(&cpu_stats->syncp, start));
+
+		stats->rx_packets += rxpackets;
+		stats->rx_bytes += rxbytes;
+		stats->tx_bytes += txbytes;
+		stats->tx_packets += txpackets;
+	}
+}
-- 
2.4.11

Re: [PATCH net-next 0/7] bridge: per-vlan stats

[Stephen Hemminger]

On Wed, 27 Apr 2016 18:18:15 +0200
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:

> Hi,
> This set adds support for bridge per-vlan statistics.
> In order to be able to dump statistics we need a way to continue
> dumping after reaching maximum size, thus patches 01-03 extend the new
> stats API with a per-device extended link stats attribute and callback
> which can save its local state and continue where it left off afterwards.
> I considered using the already existing "fill_xstats" callback but it gets
> confusing since we need to separate the linkinfo dump from the new stats
> api dump and adding a flag/argument to do that just looks messy. I don't
> think the rtnl_link_ops size is an issue, so adding these seemed like the
> cleaner approach.
> 
> Patch 05 converts the pvid to a pointer so we can consolidate the vlan
> stats accounting paths later, also allows to simplify the pvid code.
> Patches 06 and 07 add the stats support and netlink dump support
> respectively.
> I've tested this set with both old and modified iproute2, kmemleak on and
> some traffic stress tests while adding/removing vlans and ports.
> 
> Thank you,
>  Nik
> 
> Note: Jamal I haven't forgotten about the per-port per-vlan stats, I've got
> a follow-up patch that adds it. You can easily see that the infrastructure
> for private port/vlan stats is in place after this set. Though the stats
> api will need some more changes to support that.
> 
> 
> Nikolay Aleksandrov (7):
>   net: rtnetlink: allow rtnl_fill_statsinfo to save private state
>     counter
>   net: rtnetlink: allow only one idx saving stats attribute
>   net: rtnetlink: add linkxstats callbacks and attribute
>   net: constify is_skb_forwardable's arguments
>   bridge: vlan: RCUify pvid
>   bridge: vlan: learn to count
>   bridge: netlink: export per-vlan stats
> 
>  include/linux/netdevice.h      |   3 +-
>  include/net/rtnetlink.h        |  10 +++
>  include/uapi/linux/if_bridge.h |   8 +++
>  include/uapi/linux/if_link.h   |   9 +++
>  net/bridge/br_netlink.c        |  80 ++++++++++++++++++++----
>  net/bridge/br_private.h        |  32 +++++-----
>  net/bridge/br_vlan.c           | 134 +++++++++++++++++++++++++++++------------
>  net/core/dev.c                 |   2 +-
>  net/core/rtnetlink.c           |  64 +++++++++++++++++---
>  9 files changed, 266 insertions(+), 76 deletions(-)
> 

I am concerned this adds unnecessary complexity (more bugs)
and overhead (slower performance). Statistics are not free, and having
them in a convenient place maybe unnecessary duplication.

Re: [PATCH net-next 0/7] bridge: per-vlan stats

[Nikolay Aleksandrov]

On 04/27/2016 07:06 PM, Stephen Hemminger wrote:
> On Wed, 27 Apr 2016 18:18:15 +0200
> Nikolay Aleksandrov <nikolay@cumulusnetworks.com> wrote:
> 
>> Hi,
>> This set adds support for bridge per-vlan statistics.
>> In order to be able to dump statistics we need a way to continue
>> dumping after reaching maximum size, thus patches 01-03 extend the new
>> stats API with a per-device extended link stats attribute and callback
>> which can save its local state and continue where it left off afterwards.
>> I considered using the already existing "fill_xstats" callback but it gets
>> confusing since we need to separate the linkinfo dump from the new stats
>> api dump and adding a flag/argument to do that just looks messy. I don't
>> think the rtnl_link_ops size is an issue, so adding these seemed like the
>> cleaner approach.
>>
>> Patch 05 converts the pvid to a pointer so we can consolidate the vlan
>> stats accounting paths later, also allows to simplify the pvid code.
>> Patches 06 and 07 add the stats support and netlink dump support
>> respectively.
>> I've tested this set with both old and modified iproute2, kmemleak on and
>> some traffic stress tests while adding/removing vlans and ports.
>>
>> Thank you,
>>  Nik
>>
>> Note: Jamal I haven't forgotten about the per-port per-vlan stats, I've got
>> a follow-up patch that adds it. You can easily see that the infrastructure
>> for private port/vlan stats is in place after this set. Though the stats
>> api will need some more changes to support that.
>>
>>
>> Nikolay Aleksandrov (7):
>>   net: rtnetlink: allow rtnl_fill_statsinfo to save private state
>>     counter
>>   net: rtnetlink: allow only one idx saving stats attribute
>>   net: rtnetlink: add linkxstats callbacks and attribute
>>   net: constify is_skb_forwardable's arguments
>>   bridge: vlan: RCUify pvid
>>   bridge: vlan: learn to count
>>   bridge: netlink: export per-vlan stats
>>
>>  include/linux/netdevice.h      |   3 +-
>>  include/net/rtnetlink.h        |  10 +++
>>  include/uapi/linux/if_bridge.h |   8 +++
>>  include/uapi/linux/if_link.h   |   9 +++
>>  net/bridge/br_netlink.c        |  80 ++++++++++++++++++++----
>>  net/bridge/br_private.h        |  32 +++++-----
>>  net/bridge/br_vlan.c           | 134 +++++++++++++++++++++++++++++------------
>>  net/core/dev.c                 |   2 +-
>>  net/core/rtnetlink.c           |  64 +++++++++++++++++---
>>  9 files changed, 266 insertions(+), 76 deletions(-)
>>
> 
> I am concerned this adds unnecessary complexity (more bugs)
IMO the whole point in moving to a per-vlan structure from a bitmap was to
have per-vlan context and flexibility to implement functions like this.
The fast path code that is added is minimal (fetch & stats adds).
In any case I'm sure there're more per-vlan options coming, and not only from
me or Cumulus. If we won't make use of the per-vlan context, then I'd suggest
we revert to bitmaps as that's faster and more compact.

> and overhead (slower performance). Statistics are not free, and having
> them in a convenient place maybe unnecessary duplication.
> 
The performance impact is minimal as we're using per-cpu counters. If you're
concerned that even that is too much I can make this conditional on a per-vlan
flag that can be requested by the user to enable the stats, but that is overkill
for something as basic as stats in my opinion.

Thanks,
 Nik

Re: [PATCH net-next 0/7] bridge: per-vlan stats

[Nikolay Aleksandrov]

On 04/27/2016 06:18 PM, Nikolay Aleksandrov wrote:
> Hi,
> This set adds support for bridge per-vlan statistics.
> In order to be able to dump statistics we need a way to continue
> dumping after reaching maximum size, thus patches 01-03 extend the new
> stats API with a per-device extended link stats attribute and callback
> which can save its local state and continue where it left off afterwards.
> I considered using the already existing "fill_xstats" callback but it gets
> confusing since we need to separate the linkinfo dump from the new stats
> api dump and adding a flag/argument to do that just looks messy. I don't
> think the rtnl_link_ops size is an issue, so adding these seemed like the
> cleaner approach.
> 
> Patch 05 converts the pvid to a pointer so we can consolidate the vlan
> stats accounting paths later, also allows to simplify the pvid code.
> Patches 06 and 07 add the stats support and netlink dump support
> respectively.
> I've tested this set with both old and modified iproute2, kmemleak on and
> some traffic stress tests while adding/removing vlans and ports.
> 
> Thank you,
>  Nik
> 
> Note: Jamal I haven't forgotten about the per-port per-vlan stats, I've got
> a follow-up patch that adds it. You can easily see that the infrastructure
> for private port/vlan stats is in place after this set. Though the stats
> api will need some more changes to support that.
> 
> 
[snip]

Self-NAK
I'll post a v2 after a couple of days, I'd like to make some minor changes and also
address the feedback in the process.

Thanks,
 Nik