* [PATCH net-next v5 0/1] Add support for tc cookies
From: Jamal Hadi Salim @ 2017-01-22 12:51 UTC (permalink / raw)
To: davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
From: Jamal Hadi Salim <jhs@mojatatu.com>
Changes in V5:
- kill the stylistic changes
- Adopt a new structure with length-valuepointer representation
- rename some things
Changes in v4:
- move stylistic changes out into a separate patch
(and add more stylistic changes)
Changes in v3:
- use TC_ prefix for the max size
- move the cookie struct so visible only to kernel
- remove unneeded void * cast
Changes in V2:
-move from a union to a length-value representation
Jamal Hadi Salim (1):
net sched actions: Add support for user cookies
include/net/act_api.h | 1 +
include/net/pkt_cls.h | 8 ++++++++
include/uapi/linux/pkt_cls.h | 3 +++
net/sched/act_api.c | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 47 insertions(+)
--
1.9.1
^ permalink raw reply
* [PATCH net-next v5 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-22 12:51 UTC (permalink / raw)
To: davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel, Jamal Hadi Salim
In-Reply-To: <1485089498-6673-1-git-send-email-jhs@emojatatu.com>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Introduce optional 128-bit action cookie.
Like all other cookie schemes in the networking world (eg in protocols
like http or existing kernel fib protocol field, etc) the idea is to save
user state that when retrieved serves as a correlator. The kernel
_should not_ intepret it. The user can store whatever they wish in the
128 bits.
Sample exercise(showing variable length use of cookie)
.. create an accept action with cookie a1b2c3d4
sudo $TC actions add action ok index 1 cookie a1b2c3d4
.. dump all gact actions..
sudo $TC -s actions ls action gact
action order 0: gact action pass
random type none pass val 0
index 1 ref 1 bind 0 installed 5 sec used 5 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie a1b2c3d4
.. bind the accept action to a filter..
sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
... send some traffic..
$ ping 127.0.0.1 -c 3
PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
--- 127.0.0.1 ping statistics ---
3 packets transmitted, 3 received, 0% packet loss, time 2109ms
rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
... show some stats
$ sudo $TC -s actions get action gact index 1
action order 1: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 204 sec used 5 sec
Action statistics:
Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie a1b2c3d4
.. try longer cookie...
$ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
.. dump..
$ sudo $TC -s actions ls action gact
action order 1: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 204 sec used 5 sec
Action statistics:
Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
cookie 1234567890abcdef
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
include/net/act_api.h | 1 +
include/net/pkt_cls.h | 8 ++++++++
include/uapi/linux/pkt_cls.h | 3 +++
net/sched/act_api.c | 35 +++++++++++++++++++++++++++++++++++
4 files changed, 47 insertions(+)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 1d71644..cfa2ae3 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -41,6 +41,7 @@ struct tc_action {
struct rcu_head tcfa_rcu;
struct gnet_stats_basic_cpu __percpu *cpu_bstats;
struct gnet_stats_queue __percpu *cpu_qstats;
+ struct tc_cookie *act_cookie;
};
#define tcf_head common.tcfa_head
#define tcf_index common.tcfa_index
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index f0a0514..b43077e 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -515,4 +515,12 @@ struct tc_cls_bpf_offload {
u32 gen_flags;
};
+
+/* This structure holds cookie structure that is passed from user
+ * to the kernel for actions and classifiers
+ */
+struct tc_cookie {
+ u8 *data;
+ u32 len;
+};
#endif
diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index fd373eb..345551e 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -4,6 +4,8 @@
#include <linux/types.h>
#include <linux/pkt_sched.h>
+#define TC_COOKIE_MAX_SIZE 16
+
/* Action attributes */
enum {
TCA_ACT_UNSPEC,
@@ -12,6 +14,7 @@ enum {
TCA_ACT_INDEX,
TCA_ACT_STATS,
TCA_ACT_PAD,
+ TCA_ACT_COOKIE,
__TCA_ACT_MAX
};
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index cd08df9..84052630 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -24,6 +24,7 @@
#include <net/net_namespace.h>
#include <net/sock.h>
#include <net/sch_generic.h>
+#include <net/pkt_cls.h>
#include <net/act_api.h>
#include <net/netlink.h>
@@ -33,6 +34,8 @@ static void free_tcf(struct rcu_head *head)
free_percpu(p->cpu_bstats);
free_percpu(p->cpu_qstats);
+ kfree(p->act_cookie->data);
+ kfree(p->act_cookie);
kfree(p);
}
@@ -475,6 +478,12 @@ int tcf_action_destroy(struct list_head *actions, int bind)
goto nla_put_failure;
if (tcf_action_copy_stats(skb, a, 0))
goto nla_put_failure;
+ if (a->act_cookie) {
+ if (nla_put(skb, TCA_ACT_COOKIE, a->act_cookie->len,
+ a->act_cookie->data))
+ goto nla_put_failure;
+ }
+
nest = nla_nest_start(skb, TCA_OPTIONS);
if (nest == NULL)
goto nla_put_failure;
@@ -575,6 +584,32 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (err < 0)
goto err_mod;
+ if (tb[TCA_ACT_COOKIE]) {
+ int cklen = nla_len(tb[TCA_ACT_COOKIE]);
+
+ if (cklen > TC_COOKIE_MAX_SIZE) {
+ err = -EINVAL;
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+
+ a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
+ if (!a->act_cookie) {
+ err = -ENOMEM;
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+
+ a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE],
+ GFP_KERNEL);
+ if (!a->act_cookie->data) {
+ err = -ENOMEM;
+ tcf_hash_release(a, bind);
+ goto err_mod;
+ }
+ a->act_cookie->len = cklen;
+ }
+
/* module count goes up only when brand new policy is created
* if it exists and is only bound to in a_o->init() then
* ACT_P_CREATED is not returned (a zero is).
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next v5 0/1] Add support for tc cookies
From: Jamal Hadi Salim @ 2017-01-22 12:54 UTC (permalink / raw)
To: davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel
In-Reply-To: <1485089498-6673-1-git-send-email-jhs@emojatatu.com>
I removed people's reviewed/Acked because i changed the data structure
per Daniel's suggestions.
cheers,
jamal
On 17-01-22 07:51 AM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Changes in V5:
> - kill the stylistic changes
> - Adopt a new structure with length-valuepointer representation
> - rename some things
>
> Changes in v4:
> - move stylistic changes out into a separate patch
> (and add more stylistic changes)
>
> Changes in v3:
> - use TC_ prefix for the max size
> - move the cookie struct so visible only to kernel
> - remove unneeded void * cast
>
> Changes in V2:
> -move from a union to a length-value representation
>
> Jamal Hadi Salim (1):
> net sched actions: Add support for user cookies
>
> include/net/act_api.h | 1 +
> include/net/pkt_cls.h | 8 ++++++++
> include/uapi/linux/pkt_cls.h | 3 +++
> net/sched/act_api.c | 35 +++++++++++++++++++++++++++++++++++
> 4 files changed, 47 insertions(+)
>
^ permalink raw reply
* Re: [patch net-next 2/4] net/sched: Introduce sample tc action
From: Jamal Hadi Salim @ 2017-01-22 13:16 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, yotamg, idosch, eladr, nogahf, ogerlitz, geert+renesas,
stephen, xiyou.wangcong, linux, roopa, john.fastabend,
simon.horman, mrv
In-Reply-To: <1485085487-2652-3-git-send-email-jiri@resnulli.us>
On 17-01-22 06:44 AM, Jiri Pirko wrote:
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> new file mode 100644
> index 0000000..24e20e4
> --- /dev/null
> +++ b/net/sched/act_sample.c
> @@ -0,0 +1,274 @@
> +/*
> + * net/sched/act_sample.c - Packet samplig tc action
typo: "Sampling"
> +static int tcf_sample(struct sk_buff *skb, const struct tc_action *a,
> + struct tcf_result *res)
Can you rename this function because it is also the name of the data
structure? It makes it easier to grep.
I know we have this all over the place in other actions (and i hope
those are cleaned up at some point).
otherwise:
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* Re: [patch net-next 1/4] net: Introduce psample, a new genetlink channel for packet sampling
From: Jamal Hadi Salim @ 2017-01-22 13:19 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, yotamg, idosch, eladr, nogahf, ogerlitz, geert+renesas,
stephen, xiyou.wangcong, linux, roopa, john.fastabend,
simon.horman, mrv
In-Reply-To: <1485085487-2652-2-git-send-email-jiri@resnulli.us>
On 17-01-22 06:44 AM, Jiri Pirko wrote:
> From: Yotam Gigi <yotamg@mellanox.com>
>
> Add a general way for kernel modules to sample packets, without being tied
> to any specific subsystem. This netlink channel can be used by tc,
> iptables, etc. and allow to standardize packet sampling in the kernel.
>
> For every sampled packet, the psample module adds the following metadata
> fields:
>
> PSAMPLE_ATTR_IIFINDEX - the packets input ifindex, if applicable
>
> PSAMPLE_ATTR_OIFINDEX - the packet output ifindex, if applicable
>
> PSAMPLE_ATTR_ORIGSIZE - the packet's original size, in case it has been
> truncated during sampling
>
> PSAMPLE_ATTR_SAMPLE_GROUP - the packet's sample group, which is set by the
> user who initiated the sampling. This field allows the user to
> differentiate between several samplers working simultaneously and
> filter packets relevant to him
>
> PSAMPLE_ATTR_GROUP_SEQ - sequence counter of last sent packet. The
> sequence is kept for each group
>
> PSAMPLE_ATTR_SAMPLE_RATE - the sampling rate used for sampling the packets
>
> PSAMPLE_ATTR_DATA - the actual packet bits
>
> In addition, add the GET_GROUPS netlink command which allows the user to
> see the current sample groups, their refcount and sequence number. This
> command currently supports only netlink dump mode.
>
> Signed-off-by: Yotam Gigi <yotamg@mellanox.com>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Will be useful to describe in the commit log that one needs to listen
to PSAMPLE_NL_MCGRP_SAMPLE to see the samples.
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* Re: [net, 6/6] net: korina: version bump
From: Felix Fietkau @ 2017-01-22 13:21 UTC (permalink / raw)
To: Roman Yeryomin; +Cc: netdev
In-Reply-To: <CACiydbKPNZyN+_+TDkHsbg0uGR51iDcqSTAQUMKyVCJ+u9wL7w@mail.gmail.com>
On 2017-01-22 13:10, Roman Yeryomin wrote:
> On 17 January 2017 at 21:19, Roman Yeryomin <leroi.lists@gmail.com> wrote:
>> On 17 January 2017 at 20:55, Felix Fietkau <nbd@nbd.name> wrote:
>>> On 2017-01-17 18:33, Roman Yeryomin wrote:
>>>> Signed-off-by: Roman Yeryomin <roman@advem.lv>
>>>> ---
>>>> drivers/net/ethernet/korina.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/korina.c b/drivers/net/ethernet/korina.c
>>>> index 83c994f..c8fed01 100644
>>>> --- a/drivers/net/ethernet/korina.c
>>>> +++ b/drivers/net/ethernet/korina.c
>>>> @@ -66,8 +66,8 @@
>>>> #include <asm/mach-rc32434/dma_v.h>
>>>>
>>>> #define DRV_NAME "korina"
>>>> -#define DRV_VERSION "0.10"
>>>> -#define DRV_RELDATE "04Mar2008"
>>>> +#define DRV_VERSION "0.20"
>>>> +#define DRV_RELDATE "15Jan2017"
>>> I think it would make more sense to remove this version instead of
>>> bumping it. Individual driver versions are rather pointless, the kernel
>>> version is more meaningful anyway.
>>
>> OK, makes sense
>
> Actually, after thinking a bit more about this, not really...
> How about ethtool, which uses driver name and version?
> I see most ethernet drivers define some version. And it's pretty
> useful, when using backports.
> IMO, it should be kept and bumped.
I don't really care, I just wanted to point out that the exact kernel
version is a much more useful indicator, especially since not all patch
submitters do the useless version bump dance.
- Felix
^ permalink raw reply
* RE: [patch net-next 2/4] net/sched: Introduce sample tc action
From: Yotam Gigi @ 2017-01-22 13:28 UTC (permalink / raw)
To: Jamal Hadi Salim, Jiri Pirko, netdev@vger.kernel.org
Cc: davem@davemloft.net, Ido Schimmel, Elad Raz, Nogah Frankel,
Or Gerlitz, geert+renesas@glider.be, stephen@networkplumber.org,
xiyou.wangcong@gmail.com, linux@roeck-us.net,
roopa@cumulusnetworks.com, john.fastabend@gmail.com,
simon.horman@netronome.com, mrv@mojatatu.com
In-Reply-To: <52d57d84-6e0d-0ff3-5027-6dea06f4a683@mojatatu.com>
>-----Original Message-----
>From: Jamal Hadi Salim [mailto:jhs@mojatatu.com]
>Sent: Sunday, January 22, 2017 3:17 PM
>To: Jiri Pirko <jiri@resnulli.us>; netdev@vger.kernel.org
>Cc: davem@davemloft.net; Yotam Gigi <yotamg@mellanox.com>; Ido Schimmel
><idosch@mellanox.com>; Elad Raz <eladr@mellanox.com>; Nogah Frankel
><nogahf@mellanox.com>; Or Gerlitz <ogerlitz@mellanox.com>;
>geert+renesas@glider.be; stephen@networkplumber.org;
>xiyou.wangcong@gmail.com; linux@roeck-us.net; roopa@cumulusnetworks.com;
>john.fastabend@gmail.com; simon.horman@netronome.com; mrv@mojatatu.com
>Subject: Re: [patch net-next 2/4] net/sched: Introduce sample tc action
>
>On 17-01-22 06:44 AM, Jiri Pirko wrote:
>
>> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
>> new file mode 100644
>> index 0000000..24e20e4
>> --- /dev/null
>> +++ b/net/sched/act_sample.c
>> @@ -0,0 +1,274 @@
>> +/*
>> + * net/sched/act_sample.c - Packet samplig tc action
>
>typo: "Sampling"
It took me a while to see it. Will fix :)
>
>
>> +static int tcf_sample(struct sk_buff *skb, const struct tc_action *a,
>> + struct tcf_result *res)
>
>
>Can you rename this function because it is also the name of the data
>structure? It makes it easier to grep.
>I know we have this all over the place in other actions (and i hope
>those are cleaned up at some point).
OK, makes sense.
>
>otherwise:
>Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Thanks!
>
>cheers,
>jamal
^ permalink raw reply
* [PATCH iproute2 1/1] tc: distinguish Add/Replace action operations.
From: Roman Mashak @ 2017-01-22 13:55 UTC (permalink / raw)
To: stephen; +Cc: netdev, Roman Mashak, Jamal Hadi Salim
Signed-off-by: Roman Mashak <mrv@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
tc/m_action.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/tc/m_action.c b/tc/m_action.c
index bb19df8..05ef07e 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -365,12 +365,18 @@ int print_action(const struct sockaddr_nl *who,
fprintf(fp, "Flushed table ");
tab_flush = 1;
} else {
- fprintf(fp, "deleted action ");
+ fprintf(fp, "Deleted action ");
}
}
- if (n->nlmsg_type == RTM_NEWACTION)
- fprintf(fp, "Added action ");
+ if (n->nlmsg_type == RTM_NEWACTION) {
+ if ((n->nlmsg_flags & NLM_F_CREATE) &&
+ !(n->nlmsg_flags & NLM_F_REPLACE)) {
+ fprintf(fp, "Added action ");
+ } else if (n->nlmsg_flags & NLM_F_REPLACE) {
+ fprintf(fp, "Replaced action ");
+ }
+ }
tc_print_action(fp, tb[TCA_ACT_TAB]);
return 0;
--
1.9.1
^ permalink raw reply related
* Re: [patch net-next 2/4] net/sched: Introduce sample tc action
From: Roman Mashak @ 2017-01-22 14:29 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, yotamg, idosch, eladr, nogahf, ogerlitz, jhs,
geert+renesas, stephen, xiyou.wangcong, linux, roopa,
john.fastabend, simon.horman
In-Reply-To: <1485085487-2652-3-git-send-email-jiri@resnulli.us>
Jiri Pirko <jiri@resnulli.us> writes:
> From: Yotam Gigi <yotamg@mellanox.com>
>
> This action allows the user to sample traffic matched by tc classifier.
> The sampling consists of choosing packets randomly and sampling them using
> the psample module. The user can configure the psample group number, the
> sampling rate and the packet's truncation (to save kernel-user traffic).
>
[skip]
> diff --git a/include/uapi/linux/tc_act/tc_sample.h b/include/uapi/linux/tc_act/tc_sample.h
> new file mode 100644
> index 0000000..21378bc
> --- /dev/null
> +++ b/include/uapi/linux/tc_act/tc_sample.h
> @@ -0,0 +1,26 @@
> +#ifndef __LINUX_TC_SAMPLE_H
> +#define __LINUX_TC_SAMPLE_H
> +
> +#include <linux/types.h>
> +#include <linux/pkt_cls.h>
> +#include <linux/if_ether.h>
> +
> +#define TCA_ACT_SAMPLE 26
> +
> +struct tc_sample {
> + tc_gen;
> +};
> +
> +enum {
> + TCA_SAMPLE_UNSPEC,
> + TCA_SAMPLE_PARMS,
> + TCA_SAMPLE_TM,
> + TCA_SAMPLE_RATE,
> + TCA_SAMPLE_TRUNC_SIZE,
> + TCA_SAMPLE_PSAMPLE_GROUP,
> + TCA_SAMPLE_PAD,
> + __TCA_SAMPLE_MAX
> +};
Most of action implementations define TCA_X_TM attribute as 1,
and TCA_X_PARMS as 2 followed by action specific tlvs, it is better to
adhere this style in newly designed actions.
[skip]
--
Roman Mashak
^ permalink raw reply
* Re: [PATCH] net/mlx4: use rb_entry()
From: Geliang Tang @ 2017-01-22 14:42 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Yishai Hadas, netdev, linux-rdma, linux-kernel
In-Reply-To: <20170122074839.GD28570@mtr-leonro.local>
On Sun, Jan 22, 2017 at 09:48:39AM +0200, Leon Romanovsky wrote:
> On Fri, Jan 20, 2017 at 10:36:57PM +0800, Geliang Tang wrote:
> > To make the code clearer, use rb_entry() instead of container_of() to
> > deal with rbtree.
> >
> > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
>
> I don't understand completely the rationale behind this conversion.
> rb_entry == container_of, why do we need another name for it?
>
There are several *_entry macros which are defined in kernel data
structures, like list_entry, hlist_entry, rb_entry, etc. Each of them is
just another name for container_of. We use different *_entry so that we
could identify the specific type of data structure that we are dealing
with.
-Geliang
^ permalink raw reply
* Re: [RFC PATCH net-next 2/5] vxlan: make COLLECT_METADATA mode bridge friendly
From: Roopa Prabhu @ 2017-01-22 15:18 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, davem, stephen, tgraf, hannes, jbenc, pshelar, dsa, hadi
In-Reply-To: <f4c86e0a-472b-700c-98fe-f5da2eea35e6@cumulusnetworks.com>
On 1/22/17, 3:40 AM, Nikolay Aleksandrov wrote:
> On 21/01/17 06:46, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This patch series makes vxlan COLLECT_METADATA mode bridge
>> and layer2 network friendly. Vxlan COLLECT_METADATA mode today
>> solves the per-vni netdev scalability problem in l3 networks.
>> When vxlan collect metadata device participates in bridging
>> vlan to vn-segments, It can only get the vlan mapped vni in
>> the xmit tunnel dst metadata. It will need the vxlan driver to
>> continue learn, hold forwarding state and remote destination
>> information similar to how it already does for non COLLECT_METADATA
>> vxlan netdevices today.
>>
>> Changes introduced by this patch:
>> - allow learning and forwarding database state to vxlan netdev in
>> COLLECT_METADATA mode. Current behaviour is not changed
>> by default. tunnel info flag IP_TUNNEL_INFO_BRIDGE is used
>> to support the new bridge friendly mode.
>> - A single fdb table hashed by (mac, vni) to allow fdb entries with
>> multiple vnis in the same fdb table
>> - rx path already has the vni
>> - tx path expects a vni in the packet with dst_metadata
>> - prior to this series, fdb remote_dsts carried remote vni and
>> the vxlan device carrying the fdb table represented the
>> source vni. With the vxlan device now representing multiple vnis,
>> this patch adds a src vni attribute to the fdb entry. The remote
>> vni already uses NDA_VNI attribute. This patch introduces
>> NDA_SRC_VNI netlink attribute to represent the src vni in a multi
>> vni fdb table.
>>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
> [snip]
>> @@ -2173,23 +2221,29 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>> bool did_rsc = false;
>> struct vxlan_rdst *rdst, *fdst = NULL;
>> struct vxlan_fdb *f;
>> + __be32 vni = 0;
>>
>> info = skb_tunnel_info(skb);
>>
>> skb_reset_mac_header(skb);
>>
>> if (vxlan->flags & VXLAN_F_COLLECT_METADATA) {
>> - if (info && info->mode & IP_TUNNEL_INFO_TX)
>> - vxlan_xmit_one(skb, dev, NULL, false);
>> - else
>> - kfree_skb(skb);
>> - return NETDEV_TX_OK;
>> + if (info && info->mode & IP_TUNNEL_INFO_BRIDGE &&
>> + info->mode & IP_TUNNEL_INFO_TX) {
> nit: parentheses around the IP_TUNNEL_INFO_TX check
>
>> + vni = tunnel_id_to_key32(info->key.tun_id);
>> + } else {
>> + if (info && info->mode & IP_TUNNEL_INFO_TX)
> nit: parentheses around the IP_TUNNEL_INFO_TX check
ack
>> + vxlan_xmit_one(skb, dev, vni, NULL, false);
>> + else
>> + kfree_skb(skb);
>> + return NETDEV_TX_OK;
>> + }
>> }
>>
>> if (vxlan->flags & VXLAN_F_PROXY) {
>> eth = eth_hdr(skb);
>> if (ntohs(eth->h_proto) == ETH_P_ARP)
>> - return arp_reduce(dev, skb);
>> + return arp_reduce(dev, skb, vni);
>> #if IS_ENABLED(CONFIG_IPV6)
>> else if (ntohs(eth->h_proto) == ETH_P_IPV6 &&
>> pskb_may_pull(skb, sizeof(struct ipv6hdr)
>> @@ -2200,13 +2254,13 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>> msg = (struct nd_msg *)skb_transport_header(skb);
>> if (msg->icmph.icmp6_code == 0 &&
>> msg->icmph.icmp6_type == NDISC_NEIGHBOUR_SOLICITATION)
>> - return neigh_reduce(dev, skb);
>> + return neigh_reduce(dev, skb, vni);
>> }
>> #endif
>> }
>>
>> eth = eth_hdr(skb);
>> - f = vxlan_find_mac(vxlan, eth->h_dest);
>> + f = vxlan_find_mac(vxlan, eth->h_dest, vni);
>> did_rsc = false;
>>
>> if (f && (f->flags & NTF_ROUTER) && (vxlan->flags & VXLAN_F_RSC) &&
>> @@ -2214,11 +2268,11 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>> ntohs(eth->h_proto) == ETH_P_IPV6)) {
>> did_rsc = route_shortcircuit(dev, skb);
>> if (did_rsc)
>> - f = vxlan_find_mac(vxlan, eth->h_dest);
>> + f = vxlan_find_mac(vxlan, eth->h_dest, vni);
>> }
>>
>> if (f == NULL) {
>> - f = vxlan_find_mac(vxlan, all_zeros_mac);
>> + f = vxlan_find_mac(vxlan, all_zeros_mac, vni);
>> if (f == NULL) {
>> if ((vxlan->flags & VXLAN_F_L2MISS) &&
>> !is_multicast_ether_addr(eth->h_dest))
>> @@ -2239,11 +2293,11 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
>> }
>> skb1 = skb_clone(skb, GFP_ATOMIC);
>> if (skb1)
>> - vxlan_xmit_one(skb1, dev, rdst, did_rsc);
>> + vxlan_xmit_one(skb1, dev, vni, rdst, did_rsc);
>> }
>>
>> if (fdst)
>> - vxlan_xmit_one(skb, dev, fdst, did_rsc);
>> + vxlan_xmit_one(skb, dev, vni, fdst, did_rsc);
>> else
>> kfree_skb(skb);
>> return NETDEV_TX_OK;
>> @@ -2307,12 +2361,12 @@ static int vxlan_init(struct net_device *dev)
>> return 0;
>> }
>>
>> -static void vxlan_fdb_delete_default(struct vxlan_dev *vxlan)
>> +static void vxlan_fdb_delete_default(struct vxlan_dev *vxlan, __be32 vni)
>> {
>> struct vxlan_fdb *f;
>>
>> spin_lock_bh(&vxlan->hash_lock);
>> - f = __vxlan_find_mac(vxlan, all_zeros_mac);
>> + f = __vxlan_find_mac(vxlan, all_zeros_mac, vni);
>> if (f)
>> vxlan_fdb_destroy(vxlan, f);
>> spin_unlock_bh(&vxlan->hash_lock);
>> @@ -2322,7 +2376,7 @@ static void vxlan_uninit(struct net_device *dev)
>> {
>> struct vxlan_dev *vxlan = netdev_priv(dev);
>>
>> - vxlan_fdb_delete_default(vxlan);
>> + vxlan_fdb_delete_default(vxlan, vxlan->cfg.vni);
>>
>> free_percpu(dev->tstats);
>> }
>> @@ -2536,6 +2590,8 @@ static void vxlan_setup(struct net_device *dev)
>> dev->vlan_features = dev->features;
>> dev->hw_features |= NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
>> dev->hw_features |= NETIF_F_GSO_SOFTWARE;
>> + dev->hw_features |= NETIF_F_HW_VLAN_CTAG_TX;
>> + dev->features |= dev->hw_features;
>> netif_keep_dst(dev);
>> dev->priv_flags |= IFF_NO_QUEUE;
>>
>> @@ -2921,6 +2977,7 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
>> NLM_F_EXCL|NLM_F_CREATE,
>> vxlan->cfg.dst_port,
>> vxlan->default_dst.remote_vni,
>> + vxlan->default_dst.remote_vni,
>> vxlan->default_dst.remote_ifindex,
>> NTF_SELF);
>> if (err)
>> @@ -2929,7 +2986,7 @@ static int vxlan_dev_configure(struct net *src_net, struct net_device *dev,
>>
>> err = register_netdevice(dev);
>> if (err) {
>> - vxlan_fdb_delete_default(vxlan);
>> + vxlan_fdb_delete_default(vxlan, vxlan->cfg.vni);
>> return err;
>> }
>>
>> @@ -3023,19 +3080,19 @@ static int vxlan_newlink(struct net *src_net, struct net_device *dev,
>> conf.flags |= VXLAN_F_UDP_ZERO_CSUM_TX;
>>
>> if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX] &&
>> - nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
>> + !nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]))
>> conf.flags |= VXLAN_F_UDP_ZERO_CSUM6_TX;
>>
>> if (data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX] &&
>> - nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
>> + !nla_get_u8(data[IFLA_VXLAN_UDP_ZERO_CSUM6_RX]))
>> conf.flags |= VXLAN_F_UDP_ZERO_CSUM6_RX;
>>
>> if (data[IFLA_VXLAN_REMCSUM_TX] &&
>> - nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
>> + !nla_get_u8(data[IFLA_VXLAN_REMCSUM_TX]))
>> conf.flags |= VXLAN_F_REMCSUM_TX;
>>
>> if (data[IFLA_VXLAN_REMCSUM_RX] &&
>> - nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
>> + !nla_get_u8(data[IFLA_VXLAN_REMCSUM_RX]))
>> conf.flags |= VXLAN_F_REMCSUM_RX;
> Aren't these going to break user-space ?
correct... ignore these. Not intentional. these were from an incorrect merge with an earlier changelink patch i had.
Did not realize these had crept it.
thanks.
^ permalink raw reply
* Re: [RFC PATCH net-next 5/5] bridge: vlan lwt dst_metadata hooks in ingress and egress paths
From: Roopa Prabhu @ 2017-01-22 15:27 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, davem, stephen, tgraf, hannes, jbenc, pshelar, dsa, hadi
In-Reply-To: <b0977994-907d-6868-197f-b9fe25a5632b@cumulusnetworks.com>
On 1/22/17, 4:15 AM, Nikolay Aleksandrov wrote:
> On 21/01/17 06:46, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> - ingress hook:
>> - if port is a lwt tunnel port, use tunnel info in
>> attached dst_metadata to map it to a local vlan
>> - egress hook:
>> - if port is a lwt tunnel port, use tunnel info attached to
>> vlan to set dst_metadata on the skb
>>
>> CC: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>> CC'ing Nikolay for some more eyes as he has been trying to keep the
>> bridge driver fast path lite.
>>
>> net/bridge/br_input.c | 4 ++++
>> net/bridge/br_private.h | 4 ++++
>> net/bridge/br_vlan.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 63 insertions(+)
>>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 83f356f..96602a1 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -262,6 +262,10 @@ rx_handler_result_t br_handle_frame(struct sk_buff **pskb)
>> return RX_HANDLER_CONSUMED;
>>
>> p = br_port_get_rcu(skb->dev);
>> + if (p->flags & BR_LWT_VLAN) {
>> + if (br_handle_ingress_vlan_tunnel(skb, p, nbp_vlan_group_rcu(p)))
>> + goto drop;
>> + }
> Is there any reason to do this so early (perhaps netfilter?) ? If not, you can push it to the vlan __allowed_ingress
> (and rename that function to something else, it does a hundred additional things)
> and avoid this check for all packets if vlans are disabled, thus people using non-vlan filtering
> bridge won't have an additional test in their fast path
>
>
yes, forgot to mention it in the commit log. I had it close to __allowed_ingress in my first version...had to move it up here
because br_nf_pre_routing/br_nf_pre_routing_finish reset the dst...and hence already late..
^ permalink raw reply
* Re: [RFC PATCH net-next 4/5] bridge: vlan lwt and dst_metadata netlink support
From: Roopa Prabhu @ 2017-01-22 15:23 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: netdev, davem, stephen, tgraf, hannes, jbenc, pshelar, dsa, hadi
In-Reply-To: <fc3f3b38-2a97-65c9-6759-fb243d523813@cumulusnetworks.com>
On 1/22/17, 4:05 AM, Nikolay Aleksandrov wrote:
> On 21/01/17 06:46, Roopa Prabhu wrote:
>> From: Roopa Prabhu <roopa@cumulusnetworks.com>
>>
>> This patch adds support to attach per vlan tunnel info dst
>> metadata. This enables bridge driver to map vlan to tunnel_info
>> at ingress and egress
>>
>> The initial use case is vlan to vni bridging, but the api is generic
>> to extend to any tunnel_info in the future:
>> - Uapi to configure/unconfigure/dump per vlan tunnel data
>> - netlink functions to configure vlan and tunnel_info mapping
>> - Introduces bridge port flag BR_LWT_VLAN to enable attach/detach
>> dst_metadata to bridged packets on ports.
>>
>> Use case:
>> example use for this is a vxlan bridging gateway or vtep
>> which maps vlans to vn-segments (or vnis). User can configure
>> per-vlan tunnel information which the bridge driver can use
>> to bridge vlan into the corresponding tunnel.
>>
>> CC: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>> Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
>> ---
>> CC'ing Nikolay for some more eyes as he has been trying to keep the
>> bridge driver fast path lite.
>>
>> include/linux/if_bridge.h | 1 +
>> net/bridge/br_input.c | 1 +
>> net/bridge/br_netlink.c | 410 ++++++++++++++++++++++++++++++++++++++-------
>> net/bridge/br_private.h | 18 ++
>> net/bridge/br_vlan.c | 138 ++++++++++++++-
>> 5 files changed, 507 insertions(+), 61 deletions(-)
>>
>> diff --git a/include/linux/if_bridge.h b/include/linux/if_bridge.h
>> index c6587c0..36ff611 100644
>> --- a/include/linux/if_bridge.h
>> +++ b/include/linux/if_bridge.h
>> @@ -46,6 +46,7 @@ struct br_ip_list {
>> #define BR_LEARNING_SYNC BIT(9)
>> #define BR_PROXYARP_WIFI BIT(10)
>> #define BR_MCAST_FLOOD BIT(11)
>> +#define BR_LWT_VLAN BIT(12)
>>
>> #define BR_DEFAULT_AGEING_TIME (300 * HZ)
>>
>> diff --git a/net/bridge/br_input.c b/net/bridge/br_input.c
>> index 855b72f..83f356f 100644
>> --- a/net/bridge/br_input.c
>> +++ b/net/bridge/br_input.c
>> @@ -20,6 +20,7 @@
>> #include <net/arp.h>
>> #include <linux/export.h>
>> #include <linux/rculist.h>
>> +#include <net/dst_metadata.h>
>> #include "br_private.h"
>>
>> /* Hook for brouter */
>> diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
>> index 71c7453..df997ad 100644
>> --- a/net/bridge/br_netlink.c
>> +++ b/net/bridge/br_netlink.c
>> @@ -17,17 +17,30 @@
>> #include <net/net_namespace.h>
>> #include <net/sock.h>
>> #include <uapi/linux/if_bridge.h>
>> +#include <net/dst_metadata.h>
>>
>> #include "br_private.h"
>> #include "br_private_stp.h"
>>
>> -static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg,
>> - u32 filter_mask)
>> +static size_t br_get_vlan_tinfo_size(void)
>> {
>> + return nla_total_size(0) + /* nest IFLA_BRIDGE_VLAN_TUNNEL_INFO */
>> + nla_total_size(sizeof(u32)) + /* IFLA_BRIDGE_VLAN_TUNNEL_ID */
>> + nla_total_size(sizeof(u16)) + /* IFLA_BRIDGE_VLAN_TUNNEL_VID */
>> + nla_total_size(sizeof(u16)); /* IFLA_BRIDGE_VLAN_TUNNEL_FLAGS */
>> +}
>> +
>> +static int __get_num_vlan_infos(struct net_bridge_port *p,
>> + struct net_bridge_vlan_group *vg,
>> + u32 filter_mask, int *num_vtinfos)
>> +{
>> + struct net_bridge_vlan *vbegin = NULL, *vend = NULL;
>> + struct net_bridge_vlan *vtbegin = NULL, *vtend = NULL;
>> struct net_bridge_vlan *v;
>> - u16 vid_range_start = 0, vid_range_end = 0, vid_range_flags = 0;
>> + bool get_tinfos = (p && p->flags & BR_LWT_VLAN) ? true: false;
>> + bool vcontinue, vtcontinue;
>> + int num_vinfos = 0;
>> u16 flags, pvid;
>> - int num_vlans = 0;
>>
>> if (!(filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED))
>> return 0;
>> @@ -36,6 +49,8 @@ static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg,
>> /* Count number of vlan infos */
>> list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
>> flags = 0;
>> + vcontinue = false;
>> + vtcontinue = false;
>> /* only a context, bridge vlan not activated */
>> if (!br_vlan_should_use(v))
>> continue;
>> @@ -45,47 +60,79 @@ static int __get_num_vlan_infos(struct net_bridge_vlan_group *vg,
>> if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
>> flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>>
>> - if (vid_range_start == 0) {
>> - goto initvars;
>> - } else if ((v->vid - vid_range_end) == 1 &&
>> - flags == vid_range_flags) {
>> - vid_range_end = v->vid;
>> + if (!vbegin) {
>> + vbegin = v;
>> + vend = v;
>> + vcontinue = true;
>> + } else if ((v->vid - vend->vid) == 1 &&
>> + flags == vbegin->flags) {
>> + vend = v;
>> + vcontinue = true;
>> + }
>> +
>> + if (!vcontinue) {
>> + if ((vend->vid - vbegin->vid) > 0)
>> + num_vinfos += 2;
>> + else
>> + num_vinfos += 1;
>> + }
>> +
>> + if (!get_tinfos && !v->tinfo.tunnel_id)
>> continue;
>> - } else {
>> - if ((vid_range_end - vid_range_start) > 0)
>> - num_vlans += 2;
>> +
>> + if (!vtbegin) {
>> + vtbegin = v;
>> + vtend = v;
>> + vtcontinue = true;
>> + } else if ((v->vid - vtend->vid) == 1 &&
>> + vlan_tunnel_id_isrange(vtend, v)) {
>> + vtend = v;
>> + vtcontinue = true;
>> + }
>> +
>> + if (!vtcontinue) {
>> + if ((vtend->vid - vtbegin->vid) > 0)
>> + num_vtinfos += 2;
>> else
>> - num_vlans += 1;
>> + num_vtinfos += 1;
>> + vbegin = NULL;
>> + vend = NULL;
>> }
>> -initvars:
>> - vid_range_start = v->vid;
>> - vid_range_end = v->vid;
>> - vid_range_flags = flags;
>> }
>>
>> - if (vid_range_start != 0) {
>> - if ((vid_range_end - vid_range_start) > 0)
>> - num_vlans += 2;
>> + if (vbegin) {
>> + if ((vend->vid - vbegin->vid) > 0)
>> + num_vinfos += 2;
>> else
>> - num_vlans += 1;
>> + num_vinfos += 1;
>> }
>>
>> - return num_vlans;
>> + if (get_tinfos && vtbegin && vtbegin->tinfo.tunnel_id) {
>> + if ((vtend->vid - vtbegin->vid) > 0)
>> + *num_vtinfos += 2;
>> + else
>> + *num_vtinfos += 1;
>> + }
>> +
>> + return num_vinfos;
>> }
> I think this whole function should be broken into at least a few. It's really difficult
> to parse what's going on.
ack, the vlan range handling is a bit painful and getting uglier. I will see what i can do.
I have added a few more things in this area in my latest code...ie reject tunnel info sets
if the per port flag is not set and fix some error handling paths...will send out next set
early next week.
>
>>
>> -static int br_get_num_vlan_infos(struct net_bridge_vlan_group *vg,
>> - u32 filter_mask)
>> +static int br_get_num_vlan_infos(struct net_bridge_port *p,
>> + struct net_bridge_vlan_group *vg,
>> + int *num_tinfos, u32 filter_mask)
>> {
>> int num_vlans;
>>
>> if (!vg)
>> return 0;
>>
>> - if (filter_mask & RTEXT_FILTER_BRVLAN)
>> + if (filter_mask & RTEXT_FILTER_BRVLAN) {
>> + *num_tinfos = vg->num_vlans;
>> return vg->num_vlans;
>> + }
>>
>> rcu_read_lock();
>> - num_vlans = __get_num_vlan_infos(vg, filter_mask);
>> + num_vlans = __get_num_vlan_infos(p, vg, filter_mask, num_tinfos);
>> rcu_read_unlock();
>>
>> return num_vlans;
>> @@ -95,9 +142,10 @@ static size_t br_get_link_af_size_filtered(const struct net_device *dev,
>> u32 filter_mask)
>> {
>> struct net_bridge_vlan_group *vg = NULL;
>> - struct net_bridge_port *p;
>> + struct net_bridge_port *p = NULL;
>> struct net_bridge *br;
>> - int num_vlan_infos;
>> + int num_vlan_infos, num_vlan_tinfos = 0;
>> + size_t retsize = 0;
>>
>> rcu_read_lock();
>> if (br_port_exists(dev)) {
>> @@ -107,11 +155,15 @@ static size_t br_get_link_af_size_filtered(const struct net_device *dev,
>> br = netdev_priv(dev);
>> vg = br_vlan_group_rcu(br);
>> }
>> - num_vlan_infos = br_get_num_vlan_infos(vg, filter_mask);
>> + num_vlan_infos = br_get_num_vlan_infos(p, vg, &num_vlan_tinfos,
>> + filter_mask);
>> rcu_read_unlock();
>>
>> /* Each VLAN is returned in bridge_vlan_info along with flags */
>> - return num_vlan_infos * nla_total_size(sizeof(struct bridge_vlan_info));
>> + retsize = num_vlan_infos * nla_total_size(sizeof(struct bridge_vlan_info)) +
>> + num_vlan_tinfos * br_get_vlan_tinfo_size();
>> +
>> + return retsize;
>> }
>>
>> static inline size_t br_port_info_size(void)
>> @@ -191,7 +243,8 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>> nla_put_u16(skb, IFLA_BRPORT_NO, p->port_no) ||
>> nla_put_u8(skb, IFLA_BRPORT_TOPOLOGY_CHANGE_ACK,
>> p->topology_change_ack) ||
>> - nla_put_u8(skb, IFLA_BRPORT_CONFIG_PENDING, p->config_pending))
>> + nla_put_u8(skb, IFLA_BRPORT_CONFIG_PENDING, p->config_pending) ||
>> + nla_put_u8(skb, IFLA_BRPORT_LWT_VLAN, !!(p->flags & BR_LWT_VLAN)))
>> return -EMSGSIZE;
>>
>> timerval = br_timer_value(&p->message_age_timer);
>> @@ -216,6 +269,34 @@ static int br_port_fill_attrs(struct sk_buff *skb,
>> return 0;
>> }
>>
>> +static int br_fill_vlan_tinfo(struct sk_buff *skb, u16 vid,
>> + __be64 tunnel_id, u16 flags)
>> +{
>> + __be32 tid = tunnel_id_to_key32(tunnel_id);
>> + struct nlattr *tmap;
>> +
>> + tmap = nla_nest_start(skb, IFLA_BRIDGE_VLAN_TUNNEL_INFO);
>> + if (!tmap)
>> + return -EMSGSIZE;
>> + if (nla_put_u32(skb, IFLA_BRIDGE_VLAN_TUNNEL_ID,
>> + be32_to_cpu(tid)))
>> + goto nla_put_failure;
>> + if (nla_put_u16(skb, IFLA_BRIDGE_VLAN_TUNNEL_VID,
>> + vid))
>> + goto nla_put_failure;
>> + if (nla_put_u16(skb, IFLA_BRIDGE_VLAN_TUNNEL_FLAGS,
>> + flags))
>> + goto nla_put_failure;
>> + nla_nest_end(skb, tmap);
>> +
>> + return 0;
>> +
>> +nla_put_failure:
>> + nla_nest_cancel(skb, tmap);
>> +
>> + return -EMSGSIZE;
>> +}
>> +
>> static int br_fill_ifvlaninfo_range(struct sk_buff *skb, u16 vid_start,
>> u16 vid_end, u16 flags)
>> {
>> @@ -249,20 +330,24 @@ static int br_fill_ifvlaninfo_range(struct sk_buff *skb, u16 vid_start,
>> }
>>
>> static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
>> + struct net_bridge_port *p,
>> struct net_bridge_vlan_group *vg)
>> {
>> + struct net_bridge_vlan *vbegin = NULL, *vend = NULL;
>> + struct net_bridge_vlan *vtbegin = NULL, *vtend = NULL;
>> + bool fill_tinfos = (p && p->flags & BR_LWT_VLAN) ? true: false;
>> struct net_bridge_vlan *v;
>> - u16 vid_range_start = 0, vid_range_end = 0, vid_range_flags = 0;
>> + bool vcontinue, vtcontinue;
>> u16 flags, pvid;
>> - int err = 0;
>> + int err;
>>
>> - /* 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);
>> + /* Count number of vlan infos */
>> list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
>> flags = 0;
>> + vcontinue = false;
>> + vtcontinue = false;
>> + /* only a context, bridge vlan not activated */
>> if (!br_vlan_should_use(v))
>> continue;
>> if (v->vid == pvid)
>> @@ -271,44 +356,103 @@ static int br_fill_ifvlaninfo_compressed(struct sk_buff *skb,
>> if (v->flags & BRIDGE_VLAN_INFO_UNTAGGED)
>> flags |= BRIDGE_VLAN_INFO_UNTAGGED;
>>
>> - if (vid_range_start == 0) {
>> - goto initvars;
>> - } else if ((v->vid - vid_range_end) == 1 &&
>> - flags == vid_range_flags) {
>> - vid_range_end = v->vid;
>> - continue;
>> - } else {
>> - err = br_fill_ifvlaninfo_range(skb, vid_range_start,
>> - vid_range_end,
>> - vid_range_flags);
>> + if (!vbegin) {
>> + vbegin = v;
>> + vend = v;
>> + vcontinue = true;
>> + } else if ((v->vid - vend->vid) == 1 &&
>> + flags == vbegin->flags) {
>> + vend = v;
>> + vcontinue = true;
>> + }
>> +
>> + if (!vcontinue) {
>> + err = br_fill_ifvlaninfo_range(skb,
>> + vbegin->vid,
>> + vend->vid,
>> + vbegin->flags);
>> if (err)
>> return err;
>> + vbegin = vend = NULL;
>> + }
>> +
>> + if (!fill_tinfos || !v->tinfo.tunnel_id)
>> + continue;
>> +
>> + if (!vtbegin) {
>> + vtbegin = v;
>> + vtend = v;
>> + vtcontinue = true;
>> + } else if ((v->vid - vtend->vid) == 1 &&
>> + vlan_tunnel_id_isrange(vtend, v)) {
>> + vtend = v;
>> + vtcontinue = true;
>> }
>>
>> -initvars:
>> - vid_range_start = v->vid;
>> - vid_range_end = v->vid;
>> - vid_range_flags = flags;
>> + if (!vtcontinue && vtbegin->tinfo.tunnel_id) {
>> + if ((vtend->vid - vtbegin->vid) > 0) {
>> + err = br_fill_vlan_tinfo(skb, vbegin->vid,
>> + vbegin->tinfo.tunnel_id,
>> + BRIDGE_VLAN_INFO_RANGE_BEGIN);
>> + if (err)
>> + return err;
>> + err = br_fill_vlan_tinfo(skb, vend->vid,
>> + vend->tinfo.tunnel_id,
>> + BRIDGE_VLAN_INFO_RANGE_END);
>> + if (err)
>> + return err;
>> + } else {
>> + err = br_fill_vlan_tinfo(skb, vbegin->vid,
>> + vbegin->tinfo.tunnel_id,
>> + 0);
>> + if (err)
>> + return err;
>> + }
>> + vbegin = NULL;
>> + vend = NULL;
>> + }
>> }
>>
>> - if (vid_range_start != 0) {
>> - /* Call it once more to send any left over vlans */
>> - err = br_fill_ifvlaninfo_range(skb, vid_range_start,
>> - vid_range_end,
>> - vid_range_flags);
>> + if (vbegin) {
>> + err = br_fill_ifvlaninfo_range(skb, vbegin->vid,
>> + vend->vid,
>> + vbegin->flags);
>> if (err)
>> return err;
>> }
>>
>> + if (fill_tinfos && vtbegin && vtbegin->tinfo.tunnel_id) {
>> + if ((vtend->vid - vtbegin->vid) > 0) {
>> + err = br_fill_vlan_tinfo(skb, vbegin->vid,
>> + vbegin->tinfo.tunnel_id,
>> + BRIDGE_VLAN_INFO_RANGE_BEGIN);
>> + if (err)
>> + return err;
>> + err = br_fill_vlan_tinfo(skb, vend->vid,
>> + vend->tinfo.tunnel_id,
>> + BRIDGE_VLAN_INFO_RANGE_END);
>> + if (err)
>> + return err;
>> + } else {
>> + err = br_fill_vlan_tinfo(skb, vbegin->vid,
>> + vbegin->tinfo.tunnel_id, 0);
>> + if (err)
>> + return err;
>> + }
>> + }
>> +
>> return 0;
>> }
> Maybe look into breaking this one, too. It's getting huge and hard to follow..
>
>>
>> static int br_fill_ifvlaninfo(struct sk_buff *skb,
>> + struct net_bridge_port *p,
>> struct net_bridge_vlan_group *vg)
>> {
>> struct bridge_vlan_info vinfo;
>> struct net_bridge_vlan *v;
>> + bool fill_tinfos = (p && p->flags & BR_LWT_VLAN) ? true : false;
>> u16 pvid;
>> + int err;
>>
>> pvid = br_get_pvid(vg);
>> list_for_each_entry_rcu(v, &vg->vlan_list, vlist) {
>> @@ -326,6 +470,14 @@ static int br_fill_ifvlaninfo(struct sk_buff *skb,
>> if (nla_put(skb, IFLA_BRIDGE_VLAN_INFO,
>> sizeof(vinfo), &vinfo))
>> goto nla_put_failure;
>> +
>> + if (!fill_tinfos || !v->tinfo.tunnel_id)
>> + continue;
>> +
>> + err = br_fill_vlan_tinfo(skb, v->vid,
>> + v->tinfo.tunnel_id, 0);
>> + if (err)
>> + return err;
>> }
>>
>> return 0;
>> @@ -411,9 +563,9 @@ static int br_fill_ifinfo(struct sk_buff *skb,
>> goto nla_put_failure;
>> }
>> if (filter_mask & RTEXT_FILTER_BRVLAN_COMPRESSED)
>> - err = br_fill_ifvlaninfo_compressed(skb, vg);
>> + err = br_fill_ifvlaninfo_compressed(skb, port, vg);
>> else
>> - err = br_fill_ifvlaninfo(skb, vg);
>> + err = br_fill_ifvlaninfo(skb, port, vg);
>> rcu_read_unlock();
>> if (err)
>> goto nla_put_failure;
>> @@ -514,6 +666,127 @@ static int br_vlan_info(struct net_bridge *br, struct net_bridge_port *p,
>> return err;
>> }
>>
>> +static const struct nla_policy vlan_tunnel_policy[IFLA_BRIDGE_VLAN_TUNNEL_MAX + 1] = {
>> + [IFLA_BRIDGE_VLAN_TUNNEL_ID]= { .type = NLA_U32 },
>> + [IFLA_BRIDGE_VLAN_TUNNEL_VID] = { .type = NLA_U16 },
>> + [IFLA_BRIDGE_VLAN_TUNNEL_FLAGS] = { .type = NLA_U16 },
>> +};
>> +
>> +static int br_add_vlan_tunnel_info(struct net_bridge *br,
>> + struct net_bridge_port *p, int cmd,
>> + u16 vid, u32 tun_id)
>> +{
>> + int err;
>> +
>> + switch (cmd) {
>> + case RTM_SETLINK:
>> + if (p) {
>> + /* if the MASTER flag is set this will act on the global
>> + * per-VLAN entry as well
>> + */
>> + err = nbp_vlan_tunnel_info_add(p, vid, tun_id);
>> + if (err)
>> + break;
>> + } else {
>> + return -EINVAL;
>> + }
>> +
>> + break;
>> +
>> + case RTM_DELLINK:
>> + if (p)
>> + nbp_vlan_tunnel_info_delete(p, vid);
>> + else
>> + return -EINVAL;
>> + break;
>> + }
> so if (!p) return -einval in the beginning ? :-)
>
>> +
>> + return 0;
>> +}
>> +
>> +struct vtunnel_info {
>> + u32 tunid;
>> + u16 vid;
>> + u16 flags;
>> +};
>> +
>> +static int br_parse_vlan_tunnel_info(struct nlattr *attr,
>> + struct vtunnel_info *tinfo)
>> +{
>> + struct nlattr *tb[IFLA_BRIDGE_VLAN_TUNNEL_MAX + 1];
>> + u32 tun_id;
>> + u16 vid, flags;
>> + int err;
>> +
>> + err = nla_parse_nested(tb, IFLA_BRIDGE_VLAN_TUNNEL_MAX,
>> + attr, vlan_tunnel_policy);
>> + if (err < 0)
>> + return err;
>> +
>> + if (tb[IFLA_BRIDGE_VLAN_TUNNEL_ID])
>> + tun_id = nla_get_u32(tb[IFLA_BRIDGE_VLAN_TUNNEL_ID]);
>> + else
>> + return -EINVAL;
>> +
>> + if (tb[IFLA_BRIDGE_VLAN_TUNNEL_VID]) {
>> + vid = nla_get_u16(tb[IFLA_BRIDGE_VLAN_TUNNEL_VID]);
>> + if (vid >= VLAN_VID_MASK)
>> + return -ERANGE;
> !vid ||
>
>> + } else {
>> + return -EINVAL;
>> + }
> these attr checks can be moved in the beginning, usually there's a check for existence
> of the mandatory attributes, then you can continue just using them and avoid these
> "else" statements
>
>> +
>> + if (tb[IFLA_BRIDGE_VLAN_TUNNEL_FLAGS])
>> + flags = nla_get_u16(tb[IFLA_BRIDGE_VLAN_TUNNEL_FLAGS]);
>> +
>> + tinfo->tunid = tun_id;
>> + tinfo->vid = vid;
>> + tinfo->flags = flags;
>> +
>> + return 0;
>> +}
>> +
>> +static int br_process_vlan_tunnel_info(struct net_bridge *br,
>> + struct net_bridge_port *p, int cmd,
>> + struct vtunnel_info *tinfo_curr,
>> + struct vtunnel_info *tinfo_last)
>> +{
>> + int t, v;
>> + int err;
>> +
>> + if (tinfo_curr->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
>> + if (tinfo_last->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
>> + return -EINVAL;
>> + memcpy(tinfo_last, tinfo_curr, sizeof(struct vtunnel_info));
>> + } else if (tinfo_curr->flags & BRIDGE_VLAN_INFO_RANGE_END) {
>> + if (!(tinfo_last->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN))
>> + return -EINVAL;
>> + if ((tinfo_curr->vid - tinfo_last->vid) !=
>> + (tinfo_curr->tunid - tinfo_last->tunid))
>> + return -EINVAL;
>> + /* XXX: tun id and vlan id attrs must be same
>> + */
>> + t = tinfo_last->tunid;
>> + for (v = tinfo_last->vid; v <= tinfo_curr->vid; v++) {
>> + err = br_add_vlan_tunnel_info(br, p, cmd,
>> + v, t);
>> + if (err)
>> + return err;
>> + t++;
>> + }
>> + memset(tinfo_last, 0, sizeof(struct vtunnel_info));
>> + memset(tinfo_curr, 0, sizeof(struct vtunnel_info));
>> + } else {
>> + err = br_add_vlan_tunnel_info(br, p, cmd,
>> + tinfo_curr->vid,
>> + tinfo_curr->tunid);
>> + if (err)
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int br_afspec(struct net_bridge *br,
>> struct net_bridge_port *p,
>> struct nlattr *af_spec,
>> @@ -522,10 +795,30 @@ static int br_afspec(struct net_bridge *br,
>> struct bridge_vlan_info *vinfo_start = NULL;
>> struct bridge_vlan_info *vinfo = NULL;
>> struct nlattr *attr;
>> + struct vtunnel_info tinfo_last = {
>> + .tunid = 0,
>> + .vid = 0,
>> + .flags = 0};
>> + struct vtunnel_info tinfo_curr = {
>> + .tunid = 0,
>> + .vid = 0,
>> + .flags = 0};
> just { } should be enough to zero the structs
ack
>
>> int err = 0;
>> int rem;
>>
>> nla_for_each_nested(attr, af_spec, rem) {
>> + if (nla_type(attr) == IFLA_BRIDGE_VLAN_TUNNEL_INFO) {
>> + err = br_parse_vlan_tunnel_info(attr, &tinfo_curr);
>> + if (err)
>> + return err;
>> + err = br_process_vlan_tunnel_info(br, p, cmd,
>> + &tinfo_curr,
>> + &tinfo_last);
>> + if (err)
>> + return err;
>> + continue;
>> + }
>> +
>> if (nla_type(attr) != IFLA_BRIDGE_VLAN_INFO)
>> continue;
>> if (nla_len(attr) != sizeof(struct bridge_vlan_info))
>> @@ -638,6 +931,7 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
>> br_set_port_flag(p, tb, IFLA_BRPORT_MCAST_FLOOD, BR_MCAST_FLOOD);
>> br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP, BR_PROXYARP);
>> br_set_port_flag(p, tb, IFLA_BRPORT_PROXYARP_WIFI, BR_PROXYARP_WIFI);
>> + br_set_port_flag(p, tb, IFLA_BRPORT_LWT_VLAN, BR_LWT_VLAN);
>>
>> if (tb[IFLA_BRPORT_COST]) {
>> err = br_stp_set_path_cost(p, nla_get_u32(tb[IFLA_BRPORT_COST]));
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index 8ce621e..f68e360 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
>> @@ -91,6 +91,11 @@ struct br_vlan_stats {
>> struct u64_stats_sync syncp;
>> };
>>
>> +struct br_tunnel_info {
>> + __be64 tunnel_id;
>> + struct metadata_dst *tunnel_dst;
>> +};
>> +
>> /**
>> * struct net_bridge_vlan - per-vlan entry
>> *
>> @@ -113,6 +118,7 @@ struct br_vlan_stats {
>> */
>> struct net_bridge_vlan {
>> struct rhash_head vnode;
>> + struct rhash_head tnode;
>> u16 vid;
>> u16 flags;
>> struct br_vlan_stats __percpu *stats;
>> @@ -124,6 +130,9 @@ struct net_bridge_vlan {
>> atomic_t refcnt;
>> struct net_bridge_vlan *brvlan;
>> };
>> +
>> + struct br_tunnel_info tinfo;
>> +
>> struct list_head vlist;
>>
>> struct rcu_head rcu;
>> @@ -145,6 +154,7 @@ struct net_bridge_vlan {
>> */
>> struct net_bridge_vlan_group {
>> struct rhashtable vlan_hash;
>> + struct rhashtable tunnel_hash;
>> struct list_head vlan_list;
>> u16 num_vlans;
>> u16 pvid;
>> @@ -786,6 +796,14 @@ struct sk_buff *br_handle_vlan(struct net_bridge *br,
>> 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);
>> +int __vlan_tunnel_info_add(struct net_bridge_vlan_group *vg,
>> + struct net_bridge_vlan *vlan, u32 tun_id);
>> +int __vlan_tunnel_info_del(struct net_bridge_vlan_group *vg,
>> + struct net_bridge_vlan *vlan);
>> +int nbp_vlan_tunnel_info_delete(struct net_bridge_port *port, u16 vid);
>> +int nbp_vlan_tunnel_info_add(struct net_bridge_port *port, u16 vid, u32 tun_id);
>> +bool vlan_tunnel_id_isrange(struct net_bridge_vlan *v_end,
>> + struct net_bridge_vlan *v);
>>
>> static inline struct net_bridge_vlan_group *br_vlan_group(
>> const struct net_bridge *br)
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index b6de4f4..2040f08 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -3,6 +3,7 @@
>> #include <linux/rtnetlink.h>
>> #include <linux/slab.h>
>> #include <net/switchdev.h>
>> +#include <net/dst_metadata.h>
>>
>> #include "br_private.h"
>>
>> @@ -31,6 +32,31 @@ static struct net_bridge_vlan *br_vlan_lookup(struct rhashtable *tbl, u16 vid)
>> return rhashtable_lookup_fast(tbl, &vid, br_vlan_rht_params);
>> }
>>
>> +static inline int br_vlan_tunid_cmp(struct rhashtable_compare_arg *arg,
>> + const void *ptr)
>> +{
>> + const struct net_bridge_vlan *vle = ptr;
>> + __be64 tunid = *(__be64 *)arg->key;
>> +
>> + return vle->tinfo.tunnel_id != tunid;
>> +}
>> +
>> +static const struct rhashtable_params br_vlan_tunnel_rht_params = {
>> + .head_offset = offsetof(struct net_bridge_vlan, tnode),
>> + .key_offset = offsetof(struct net_bridge_vlan, tinfo.tunnel_id),
>> + .key_len = sizeof(__be64),
>> + .nelem_hint = 3,
>> + .locks_mul = 1,
>> + .obj_cmpfn = br_vlan_tunid_cmp,
>> + .automatic_shrinking = true,
>> +};
>> +
>> +static struct net_bridge_vlan *br_vlan_tunnel_lookup(struct rhashtable *tbl,
>> + u64 tunnel_id)
>> +{
>> + return rhashtable_lookup_fast(tbl, &tunnel_id, br_vlan_tunnel_rht_params);
>> +}
>> +
>> static void __vlan_add_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>> {
>> if (vg->pvid == vid)
>> @@ -325,6 +351,7 @@ static void __vlan_group_free(struct net_bridge_vlan_group *vg)
>> {
>> WARN_ON(!list_empty(&vg->vlan_list));
>> rhashtable_destroy(&vg->vlan_hash);
>> + rhashtable_destroy(&vg->tunnel_hash);
>> kfree(vg);
>> }
>>
>> @@ -613,6 +640,8 @@ int br_vlan_delete(struct net_bridge *br, u16 vid)
>> br_fdb_find_delete_local(br, NULL, br->dev->dev_addr, vid);
>> br_fdb_delete_by_port(br, NULL, vid, 0);
>>
>> + __vlan_tunnel_info_del(vg, v);
>> +
>> return __vlan_del(v);
>> }
>>
>> @@ -918,6 +947,9 @@ int br_vlan_init(struct net_bridge *br)
>> ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
>> if (ret)
>> goto err_rhtbl;
>> + ret = rhashtable_init(&vg->tunnel_hash, &br_vlan_tunnel_rht_params);
>> + if (ret)
>> + goto err_rhtbl2;
>> INIT_LIST_HEAD(&vg->vlan_list);
>> br->vlan_proto = htons(ETH_P_8021Q);
>> br->default_pvid = 1;
>> @@ -932,6 +964,8 @@ int br_vlan_init(struct net_bridge *br)
>> return ret;
>>
>> err_vlan_add:
>> + rhashtable_destroy(&vg->tunnel_hash);
>> +err_rhtbl2:
>> rhashtable_destroy(&vg->vlan_hash);
>> err_rhtbl:
>> kfree(vg);
>> @@ -960,7 +994,10 @@ int nbp_vlan_init(struct net_bridge_port *p)
>>
>> ret = rhashtable_init(&vg->vlan_hash, &br_vlan_rht_params);
>> if (ret)
>> - goto err_rhtbl;
>> + goto err_rhtbl1;
>> + ret = rhashtable_init(&vg->tunnel_hash, &br_vlan_tunnel_rht_params);
>> + if (ret)
>> + goto err_rhtbl2;
>> INIT_LIST_HEAD(&vg->vlan_list);
>> rcu_assign_pointer(p->vlgrp, vg);
>> if (p->br->default_pvid) {
>> @@ -976,9 +1013,11 @@ int nbp_vlan_init(struct net_bridge_port *p)
>> err_vlan_add:
>> RCU_INIT_POINTER(p->vlgrp, NULL);
>> synchronize_rcu();
>> - rhashtable_destroy(&vg->vlan_hash);
>> + rhashtable_destroy(&vg->tunnel_hash);
>> err_vlan_enabled:
>> -err_rhtbl:
>> +err_rhtbl2:
>> + rhashtable_destroy(&vg->vlan_hash);
>> +err_rhtbl1:
>> kfree(vg);
>>
>> goto out;
>> @@ -1081,3 +1120,96 @@ void br_vlan_get_stats(const struct net_bridge_vlan *v,
>> stats->tx_packets += txpackets;
>> }
>> }
>> +
>> +bool vlan_tunnel_id_isrange(struct net_bridge_vlan *v_end,
>> + struct net_bridge_vlan *v)
>> +{
>> + /* XXX: check other tunnel attributes */
>> + return (be32_to_cpu(tunnel_id_to_key32(v_end->tinfo.tunnel_id)) -
>> + be32_to_cpu(tunnel_id_to_key32(v->tinfo.tunnel_id)) == 1);
>> +}
>> +
>> +int __vlan_tunnel_info_add(struct net_bridge_vlan_group *vg,
>> + struct net_bridge_vlan *vlan, u32 tun_id)
>> +{
>> + struct metadata_dst *metadata = NULL;
>> + __be64 key = key32_to_tunnel_id(cpu_to_be32(tun_id));
>> + int err;
>> +
>> + if (vlan->tinfo.tunnel_dst)
>> + return -EEXIST;
>> +
>> + metadata = __ip_tun_set_dst(0, 0, 0, 0, 0, TUNNEL_KEY,
>> + key, 0);
>> + if (!metadata)
>> + return -EINVAL;
>> +
>> + metadata->u.tun_info.mode |= IP_TUNNEL_INFO_TX | IP_TUNNEL_INFO_BRIDGE;
>> + vlan->tinfo.tunnel_dst = metadata;
>> + vlan->tinfo.tunnel_id = key;
>> +
>> + err = rhashtable_lookup_insert_fast(&vg->tunnel_hash, &vlan->tnode,
>> + br_vlan_tunnel_rht_params);
>> + if (err)
>> + goto out;
>> +
>> + return 0;
>> +out:
>> + dst_release(&vlan->tinfo.tunnel_dst->dst);
>> +
>> + return err;
>> +}
>> +
>> +int __vlan_tunnel_info_del(struct net_bridge_vlan_group *vg,
>> + struct net_bridge_vlan *vlan)
>> +{
>> + if (vlan->tinfo.tunnel_dst) {
>> + vlan->tinfo.tunnel_id = 0;
>> + dst_release(&vlan->tinfo.tunnel_dst->dst);
>> +
>> + rhashtable_remove_fast(&vg->tunnel_hash, &vlan->vnode,
>> + br_vlan_tunnel_rht_params);
>> + }
>> +
>> + return 0;
>> +}
> I think all of these should be static, if I haven't missed something I don't see them
> being used anywhere else.
yep, moved most of them to static....in the latest version.
>
>> +
>> +/* Must be protected by RTNL.
>> + * Must be called with vid in range from 1 to 4094 inclusive.
>> + */
>> +int nbp_vlan_tunnel_info_add(struct net_bridge_port *port, u16 vid, u32 tun_id)
>> +{
>> + struct net_bridge_vlan_group *vg;
>> + struct net_bridge_vlan *vlan;
>> +
>> + ASSERT_RTNL();
>> +
>> + vg = nbp_vlan_group(port);
>> + vlan = br_vlan_find(vg, vid);
>> + if (!vlan)
>> + return -EINVAL;
>> +
>> + __vlan_tunnel_info_add(vg, vlan, tun_id);
>> +
>> + return 0;
>> +}
>> +
>> +/* Must be protected by RTNL.
>> + * Must be called with vid in range from 1 to 4094 inclusive.
>> + */
>> +int nbp_vlan_tunnel_info_delete(struct net_bridge_port *port, u16 vid)
>> +{
>> + struct net_bridge_vlan_group *vg;
>> + struct net_bridge_vlan *v;
>> +
>> + ASSERT_RTNL();
>> +
>> + vg = nbp_vlan_group(port);
>> + v = br_vlan_find(vg, vid);
>> + if (!v)
>> + return -ENOENT;
>> +
>> + __vlan_tunnel_info_del(vg, v);
>> +
>> + return 0;
>> +}
>>
thanks
^ permalink raw reply
* RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
From: Nogah Frankel @ 2017-01-22 15:34 UTC (permalink / raw)
To: Stephen Hemminger, Roopa Prabhu
Cc: netdev@vger.kernel.org, roszenrami@gmail.com, Jiri Pirko,
Ido Schimmel, Elad Raz, Yotam Gigi, Or Gerlitz
In-Reply-To: <20170119111105.481dcc36@xeon-e3>
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, January 19, 2017 9:11 PM
> To: Roopa Prabhu <roopa@cumulusnetworks.com>
> Cc: Nogah Frankel <nogahf@mellanox.com>; netdev@vger.kernel.org;
> roszenrami@gmail.com; Jiri Pirko <jiri@mellanox.com>; Ido Schimmel
> <idosch@mellanox.com>; Elad Raz <eladr@mellanox.com>; Yotam Gigi
> <yotamg@mellanox.com>; Or Gerlitz <ogerlitz@mellanox.com>
> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended statistics
>
> On Thu, 19 Jan 2017 08:06:21 -0800
> Roopa Prabhu <roopa@cumulusnetworks.com> wrote:
>
> > On 1/19/17, 7:21 AM, Nogah Frankel wrote:
> > >> -----Original Message-----
> > >> From: Nogah Frankel
> > >> Sent: Sunday, January 15, 2017 3:55 PM
> > >> To: 'Stephen Hemminger' <stephen@networkplumber.org>
> > >> Cc: netdev@vger.kernel.org; roszenrami@gmail.com;
> roopa@cumulusnetworks.com; Jiri
> > >> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> > >> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > >> <ogerlitz@mellanox.com>
> > >> Subject: RE: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended
> statistics
> > >>
> > >>
> > >>
> > >>> -----Original Message-----
> > >>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > >>> Sent: Friday, January 13, 2017 3:44 AM
> > >>> To: Nogah Frankel <nogahf@mellanox.com>
> > >>> Cc: netdev@vger.kernel.org; roszenrami@gmail.com;
> roopa@cumulusnetworks.com;
> > >> Jiri
> > >>> Pirko <jiri@mellanox.com>; Ido Schimmel <idosch@mellanox.com>; Elad Raz
> > >>> <eladr@mellanox.com>; Yotam Gigi <yotamg@mellanox.com>; Or Gerlitz
> > >>> <ogerlitz@mellanox.com>
> > >>> Subject: Re: [PATCH iproute2 v4 3/4] ifstat: Add 64 bits based stats to extended
> statistics
> > >>>
> > >>> On Thu, 12 Jan 2017 15:49:50 +0200
> > >>> Nogah Frankel <nogahf@mellanox.com> wrote:
> > >>>
> > >>>> The default stats for ifstat are 32 bits based.
> > >>>> The kernel supports 64 bits based stats. (They are returned in struct
> > >>>> rtnl_link_stats64 which is an exact copy of struct rtnl_link_stats, in
> > >>>> which the "normal" stats are returned, but with fields of u64 instead of
> > >>>> u32). This patch adds them as an extended stats.
> > >>>>
> > >>>> It is read with filter type IFLA_STATS_LINK_64 and no sub type.
> > >>>>
> > >>>> It is under the name 64bits
> > >>>> (or any shorten of it as "64")
> > >>>>
> > >>>> For example:
> > >>>> ifstat -x 64bit
> > >>>>
> > >>>> Signed-off-by: Nogah Frankel <nogahf@mellanox.com>
> > >>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> > >>> Other commands (like ip link) always use the 64 bit statistics if available
> > >>> from the device. I see no reason that ifstat needs to be different.
> > >>>
> > >> Do you mean to change the default ifstat results to be 64 bits based?
> > >> I tried it in the first version, but Roopa commented that it was not a good idea.
> > >> She said they tried it in the past and it caused backward compatibilities problems.
> > >> (Or maybe I didn't understand correctly)
> > > So, can I leave the default ifstat results to be 32 bits based, for the time being?
> > >
> > From past discussions: Moving the default to 64bit has compat issues with the old
> history file.
> > There is a way to make it work by using a new file header (to indicate that it is 64 bit) in
> > a freshly created history file and also check this header before dumping stats into the
> history file.
> > ie maintain backward compat without introducing a new option. It is doable.
> >
> > One approach is, you can drop the 64bit option from this series and
> > try updating the default to 64 bit (with compat handling code) in a later series.
I think I will take your suggestion to drop the 64 bits from this series.
Hopefully, I'll return to it in some later series in the future.
Thanks
>
> The ifstat code could do conversion based on file size.
>
> if (history_file_is_32bit()) {
> printf("converting to 64 bit format\n");
> ...
> }
>
>
^ permalink raw reply
* [PATCH] Documentation: net: phy: improve explanation when to specify the PHY ID
From: Martin Blumenstingl @ 2017-01-22 16:41 UTC (permalink / raw)
To: andrew-g2DYL2Zd6BY, devicetree-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, davem-fT/PcQaiUtIeIZ0/mPfg9Q,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w
Cc: mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
Martin Blumenstingl
The old description basically read like "ethernet-phy-idAAAA.BBBB" can
be specified when you know the actual PHY ID. However, specifying this
has a side-effect: it forces Linux to bind to a certain PHY driver (the
one that matches the ID given in the compatible string), ignoring the ID
which is reported by the actual PHY.
Whenever a device is shipped with (multiple) different PHYs during it's
production lifetime then explicitly specifying
"ethernet-phy-idAAAA.BBBB" could break certain revisions of that device.
Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
---
Thanks to Andrew Lunn for pointing the documentation issue out to me in:
http://lists.infradead.org/pipermail/linux-amlogic/2017-January/002141.html
Documentation/devicetree/bindings/net/phy.txt | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index ff1bc4b1bb3b..fb5056b22685 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -19,8 +19,9 @@ Optional Properties:
specifications. If neither of these are specified, the default is to
assume clause 22.
- If the phy's identifier is known then the list may contain an entry
- of the form: "ethernet-phy-idAAAA.BBBB" where
+ If the PHY reports an incorrect ID (or none at all) then the
+ "compatible" list may contain an entry with the correct PHY ID in the
+ form: "ethernet-phy-idAAAA.BBBB" where
AAAA - The value of the 16 bit Phy Identifier 1 register as
4 hex digits. This is the chip vendor OUI bits 3:18
BBBB - The value of the 16 bit Phy Identifier 2 register as
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] net: mvneta: implement .set_wol and .get_wol
From: Andrew Lunn @ 2017-01-22 16:49 UTC (permalink / raw)
To: Jingju Hou; +Cc: davem, thomas.petazzoni, netdev
In-Reply-To: <1485079590-6392-1-git-send-email-houjingj@marvell.com>
On Sun, Jan 22, 2017 at 06:06:30PM +0800, Jingju Hou wrote:
> Signed-off-by: Jingju Hou <houjingj@marvell.com>
Hi Jingju
Please include a real comment here. Something like:
The mvneta itself does not support WOL, but the PHY might. So pass the
calls to the PHY.
It also looks like you are patching an old kernel. Network patches
like this need to be against net-next. You should also include
net-next in the subject line.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH] Documentation: net: phy: improve explanation when to specify the PHY ID
From: Andrew Lunn @ 2017-01-22 16:57 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, f.fainelli-Re5JQEeQqe8AvxtiuMwx3w,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <20170122164132.13967-1-martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
On Sun, Jan 22, 2017 at 05:41:32PM +0100, Martin Blumenstingl wrote:
> The old description basically read like "ethernet-phy-idAAAA.BBBB" can
> be specified when you know the actual PHY ID. However, specifying this
> has a side-effect: it forces Linux to bind to a certain PHY driver (the
> one that matches the ID given in the compatible string), ignoring the ID
> which is reported by the actual PHY.
> Whenever a device is shipped with (multiple) different PHYs during it's
> production lifetime then explicitly specifying
> "ethernet-phy-idAAAA.BBBB" could break certain revisions of that device.
>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Reviewed-by: Andrew Lunn <andrew-g2DYL2Zd6BY@public.gmane.org>
Thanks
Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] net/mlx5e: Do not recycle pages from emergency reserve
From: Tom Herbert @ 2017-01-22 17:50 UTC (permalink / raw)
To: Saeed Mahameed
Cc: kernel netdev, Saeed Mahameed, netdev, Tariq Toukan, Davem,
Eric Dumazet, Jesper Dangaard Brouer
In-Reply-To: <CALzJLG9JEorLwV48H3m0vQ1+VDnpYbKhiBoUw+WW-JFouJoj1g@mail.gmail.com>
On Sat, Jan 21, 2017 at 12:31 PM, Saeed Mahameed
<saeedm@dev.mellanox.co.il> wrote:
> On Sat, Jan 21, 2017 at 9:12 PM, kernel netdev <netdev@brouer.com> wrote:
>>
>>
>> Den 21. jan. 2017 7.10 PM skrev "Tom Herbert" <tom@herbertland.com>:
>>
>> On Thu, Jan 19, 2017 at 11:14 AM, Saeed Mahameed
>> <saeedm@dev.mellanox.co.il> wrote:
>>> On Thu, Jan 19, 2017 at 9:03 AM, Eric Dumazet <eric.dumazet@gmail.com>
>>> wrote:
>>>> From: Eric Dumazet <edumazet@google.com>
>>>>
>>>> A driver using dev_alloc_page() must not reuse a page allocated from
>>>> emergency memory reserve.
>>>>
>>>> Otherwise all packets using this page will be immediately dropped,
>>>> unless for very specific sockets having SOCK_MEMALLOC bit set.
>>>>
>>>> This issue might be hard to debug, because only a fraction of received
>>>> packets would be dropped.
>>>
>>> Hi Eric,
>>>
>>> When you say reuse, you mean point to the same page from several SKBs ?
>>>
>>> Because in our page cache implementation we don't reuse pages that
>>> already passed to the stack,
>>> we just keep them in the page cache until the ref count drop back to
>>> one, so we recycle them (i,e they will be re-used only when no one
>>> else is using them).
>>>
>> Saeed,
>>
>> Speaking of the mlx page cache can we remove this or a least make it
>> optional to use. It is another example of complex functionality being
>> put into drivers that makes things like backports more complicated and
>
> Re complexity, I am not sure the mlx page cache is that complex,
> we just wrap alloc_page/put_page with our own page cache calls.
> Roughly the page cache implementation is 200-300 LOC tops all concentrated
> in one place in the code.
>
Taken as part of the RX buffer management code the whole thing in very
complicated and seems to be completely bereft of any comments in the
code as to how things are supposed to work.
>> provide at best some marginal value. In the case of the mlx5e cache
>> code the results from pktgen really weren't very impressive in the
>> first place. Also, the cache suffers from HOL blocking where we can
>
> Well, with pktgen you won't notice a huge improvement since the pages are freed
> in the stack directly from our rx receive handler (gro_receive), those
> pages will go back to the page allocator and get requested immediately
> again from the driver (no stress).
>
> The real improvements are seen when you really stress the page allocator with
> real uses cases such as TCP/UDP with user applications, where the
> pages are held longer than the driver needs, the page cache in this
> case will play an important role of reducing the stress
> on the page allocater since with those use cases we are juggling with
> more pages than pktgen could.
>
> With more stress (#cores TCP streams) our humble page cache some times
> can't hold ! and it will get full fast enough and will fail to recycle
> for a huge percentage of the driver pages requests.
>
> Before our own page cache we used dev_alloc_skb which used its own
> cache "page_frag_cache" and it worked nice enough. So i don't really
> recommend removing the page cache, until we have
> a generic RX page cache solution for all device drivers.
>
>> block the whole cache due to an outstanding reference on just one page
>> (something that you wouldn't see in pktgen but is likely to happen in
>> real applications).
>>
>
> Re the HOL issue, we have some upcoming patches that would drastically
> improve the HOL blocking issue (we will simple swap the HOL on every
> sample).
>
>>
>> (Send from phone in car)
>>
>
> Driver Safe :) ..
>
>> To Tom, have you measured the effect of this page cache? Before claiming it
>> is ineffective.
>>
>> My previous measurements show approx 20℅ speedup on a UDP test with delivery
>> to remote CPU.
>>
>> Removing the cache would of cause be a good usecase for speeding up the page
>> allocator (PCP). Which Mel Gorman and me are working on. AFAIK current page
>> order0 cost 240 cycles. Mel have reduced til to 180, and without NUMA 150
>> cycles. And with bulking this can be amortized to 80 cycles.
>>
>
> Are you trying to say that we won't need the cache if you manage to
> deliver those optimizations ?
> can you compare those optimizations with the page_frag_cache from
> dev_alloc_skb ?
>
>> --Jesper
>>
^ permalink raw reply
* Re: [PATCH net-next v5 1/1] net sched actions: Add support for user cookies
From: Florian Fainelli @ 2017-01-22 18:13 UTC (permalink / raw)
To: Jamal Hadi Salim, davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel
In-Reply-To: <1485089498-6673-2-git-send-email-jhs@emojatatu.com>
On 01/22/2017 04:51 AM, Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Introduce optional 128-bit action cookie.
> Like all other cookie schemes in the networking world (eg in protocols
> like http or existing kernel fib protocol field, etc) the idea is to save
> user state that when retrieved serves as a correlator. The kernel
> _should not_ intepret it. The user can store whatever they wish in the
> 128 bits.
>
> Sample exercise(showing variable length use of cookie)
>
> .. create an accept action with cookie a1b2c3d4
> sudo $TC actions add action ok index 1 cookie a1b2c3d4
>
> .. dump all gact actions..
> sudo $TC -s actions ls action gact
>
> action order 0: gact action pass
> random type none pass val 0
> index 1 ref 1 bind 0 installed 5 sec used 5 sec
> Action statistics:
> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie a1b2c3d4
>
> .. bind the accept action to a filter..
> sudo $TC filter add dev lo parent ffff: protocol ip prio 1 \
> u32 match ip dst 127.0.0.1/32 flowid 1:1 action gact index 1
>
> ... send some traffic..
> $ ping 127.0.0.1 -c 3
> PING 127.0.0.1 (127.0.0.1) 56(84) bytes of data.
> 64 bytes from 127.0.0.1: icmp_seq=1 ttl=64 time=0.020 ms
> 64 bytes from 127.0.0.1: icmp_seq=2 ttl=64 time=0.027 ms
> 64 bytes from 127.0.0.1: icmp_seq=3 ttl=64 time=0.038 ms
>
> --- 127.0.0.1 ping statistics ---
> 3 packets transmitted, 3 received, 0% packet loss, time 2109ms
> rtt min/avg/max/mdev = 0.020/0.028/0.038/0.008 ms 1
>
> ... show some stats
> $ sudo $TC -s actions get action gact index 1
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 204 sec used 5 sec
> Action statistics:
> Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie a1b2c3d4
>
> .. try longer cookie...
> $ sudo $TC actions replace action ok index 1 cookie 1234567890abcdef
> .. dump..
> $ sudo $TC -s actions ls action gact
>
> action order 1: gact action pass
> random type none pass val 0
> index 1 ref 2 bind 1 installed 204 sec used 5 sec
> Action statistics:
> Sent 12168 bytes 164 pkt (dropped 0, overlimits 0 requeues 0)
> backlog 0b 0p requeues 0
> cookie 1234567890abcdef
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> + a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE],
> + GFP_KERNEL);
> + if (!a->act_cookie->data) {
> + err = -ENOMEM;
> + tcf_hash_release(a, bind);
> + goto err_mod;
> + }
Are not you leaking a->act_cookie here in case nla_memdup() fails here?
--
Florian
^ permalink raw reply
* Re: [PATCH net] net/mlx5e: Do not recycle pages from emergency reserve
From: Tom Herbert @ 2017-01-22 18:09 UTC (permalink / raw)
To: kernel netdev
Cc: Saeed Mahameed, netdev, Tariq Toukan, Davem, Eric Dumazet,
Saeed Mahameed, Jesper Dangaard Brouer
In-Reply-To: <CAPe+=50xs=MMqxHPgTiGa3qytL7633Cygf_vBWuAuq=ceLyacg@mail.gmail.com>
On Sat, Jan 21, 2017 at 11:12 AM, kernel netdev <netdev@brouer.com> wrote:
>
>
> Den 21. jan. 2017 7.10 PM skrev "Tom Herbert" <tom@herbertland.com>:
>
> On Thu, Jan 19, 2017 at 11:14 AM, Saeed Mahameed
> <saeedm@dev.mellanox.co.il> wrote:
>> On Thu, Jan 19, 2017 at 9:03 AM, Eric Dumazet <eric.dumazet@gmail.com>
>> wrote:
>>> From: Eric Dumazet <edumazet@google.com>
>>>
>>> A driver using dev_alloc_page() must not reuse a page allocated from
>>> emergency memory reserve.
>>>
>>> Otherwise all packets using this page will be immediately dropped,
>>> unless for very specific sockets having SOCK_MEMALLOC bit set.
>>>
>>> This issue might be hard to debug, because only a fraction of received
>>> packets would be dropped.
>>
>> Hi Eric,
>>
>> When you say reuse, you mean point to the same page from several SKBs ?
>>
>> Because in our page cache implementation we don't reuse pages that
>> already passed to the stack,
>> we just keep them in the page cache until the ref count drop back to
>> one, so we recycle them (i,e they will be re-used only when no one
>> else is using them).
>>
> Saeed,
>
> Speaking of the mlx page cache can we remove this or a least make it
> optional to use. It is another example of complex functionality being
> put into drivers that makes things like backports more complicated and
> provide at best some marginal value. In the case of the mlx5e cache
> code the results from pktgen really weren't very impressive in the
> first place. Also, the cache suffers from HOL blocking where we can
> block the whole cache due to an outstanding reference on just one page
> (something that you wouldn't see in pktgen but is likely to happen in
> real applications).
>
>
> (Send from phone in car)
>
> To Tom, have you measured the effect of this page cache? Before claiming it
> is ineffective.
No, I have not. TBH, I have most of the past few weeks trying to debug
a backport of the code from 4.9 to 4.6. Until we have a working
backport performance is immaterial for our purposes. Unfortunately, we
are seeing some issues: the checksum faults I described previously and
crashes on bad page refcns which are presumably being caused by the
logic in RX buffer processing. This is why I am now having to dissect
the code and trying to disable things like the page cache that are not
essential functionality.
In any case the HOL blocking issue is obvious from reading the code
which and implies bimodal behavior-- we don't need a test for that to
know it's a bad as we've see the bad effects of that in many other
contexts.
>
> My previous measurements show approx 20℅ speedup on a UDP test with delivery
> to remote CPU.
>
> Removing the cache would of cause be a good usecase for speeding up the page
> allocator (PCP). Which Mel Gorman and me are working on. AFAIK current page
> order0 cost 240 cycles. Mel have reduced til to 180, and without NUMA 150
> cycles. And with bulking this can be amortized to 80 cycles.
>
That would be great. If only I had a nickel for every time someone
started working on a driver and came the conclusion that they need to
do a custom memory allocator because the kernel allocator is so
inefficient!
Tom
> --Jesper
>
^ permalink raw reply
* Re: [PATCH net-next v5 1/1] net sched actions: Add support for user cookies
From: Jamal Hadi Salim @ 2017-01-22 18:57 UTC (permalink / raw)
To: Florian Fainelli, davem
Cc: netdev, jiri, paulb, john.fastabend, simon.horman, mrv, hadarh,
ogerlitz, roid, xiyou.wangcong, daniel
In-Reply-To: <11da08ed-32b3-2a9f-8ba4-54b346cabecc@gmail.com>
On 17-01-22 01:13 PM, Florian Fainelli wrote:
>
>
>
>> + a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE],
>> + GFP_KERNEL);
>> + if (!a->act_cookie->data) {
>> + err = -ENOMEM;
>> + tcf_hash_release(a, bind);
>> + goto err_mod;
>> + }
>
> Are not you leaking a->act_cookie here in case nla_memdup() fails here?
>
yes, I am. Thanks for catching this. V6 coming up.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH] net/mlx4: use rb_entry()
From: Leon Romanovsky @ 2017-01-22 19:05 UTC (permalink / raw)
To: Geliang Tang; +Cc: Yishai Hadas, netdev, linux-rdma, linux-kernel
In-Reply-To: <20170122144225.lvkakc7db7fs2xpq@ThinkPad>
[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]
On Sun, Jan 22, 2017 at 10:42:25PM +0800, Geliang Tang wrote:
> On Sun, Jan 22, 2017 at 09:48:39AM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 20, 2017 at 10:36:57PM +0800, Geliang Tang wrote:
> > > To make the code clearer, use rb_entry() instead of container_of() to
> > > deal with rbtree.
> > >
> > > Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> > > ---
> > > drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > I don't understand completely the rationale behind this conversion.
> > rb_entry == container_of, why do we need another name for it?
> >
>
> There are several *_entry macros which are defined in kernel data
> structures, like list_entry, hlist_entry, rb_entry, etc. Each of them is
> just another name for container_of. We use different *_entry so that we
> could identify the specific type of data structure that we are dealing
> with.
Your proposed patch doesn't support the importance of such knowledge for
rb_entry. The list_entry case is totally different, because you perform
operation on it.
Anyway, It doesn't matter.
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 0/3] sh_eth: E-DMAC interrupt mask cleanups
From: Sergei Shtylyov @ 2017-01-22 19:17 UTC (permalink / raw)
To: netdev; +Cc: linux-renesas-soc
Hello.
Here's a set of 3 patches against DaveM's 'net-next.git' repo. The main goal
of this set is to stop using the bare numbers for the E-DMAC interrupt masks.
[1/3] sh_eth: rename EESIPR bits
[2/3] sh_eth: add missing EESIPR bits
[3/3] sh_eth: stop using bare numbers for EESIPR values
MBR, Sergei
^ permalink raw reply
* [PATCH 1/3] sh_eth: rename EESIPR bits
From: Sergei Shtylyov @ 2017-01-22 19:18 UTC (permalink / raw)
To: netdev; +Cc: linux-renesas-soc
In-Reply-To: <1636105.gp4aQxGd5K@wasted.cogentembedded.com>
Since the commit b0ca2a21f769 ("sh_eth: Add support of SH7763 to sh_eth")
the *enum* declaring the EESIPR bits (interrupt mask) went out of sync with
the *enum* declaring the EESR bits (interrupt status) WRT bit naming and
formatting. I'd like to restore the consistency by using EESIPR as the bit
name prefix, renaming the *enum* to EESIPR_BIT, and (finally) renaming the
bits according to the available Renesas SH77{34|63} manuals...
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
drivers/net/ethernet/renesas/sh_eth.c | 22 ++++++++++----------
drivers/net/ethernet/renesas/sh_eth.h | 36 +++++++++++++++++++++-------------
2 files changed, 34 insertions(+), 24 deletions(-)
Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
===================================================================
--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net-next/drivers/net/ethernet/renesas/sh_eth.c
@@ -556,7 +556,7 @@ static struct sh_eth_cpu_data r8a7740_da
.ecsr_value = ECSR_ICD | ECSR_MPD,
.ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003fffff,
.tx_check = EESR_TC1 | EESR_FTC,
.eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
@@ -702,7 +702,7 @@ static struct sh_eth_cpu_data sh7757_dat
.register_type = SH_ETH_REG_FAST_SH4,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003fffff,
.tx_check = EESR_FTC | EESR_CND | EESR_DLC | EESR_CD | EESR_RTO,
.eesr_err_check = EESR_TWB | EESR_TABT | EESR_RABT | EESR_RFE |
@@ -769,7 +769,7 @@ static struct sh_eth_cpu_data sh7757_dat
.ecsr_value = ECSR_ICD | ECSR_MPD,
.ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003fffff,
.tx_check = EESR_TC1 | EESR_FTC,
.eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
@@ -800,7 +800,7 @@ static struct sh_eth_cpu_data sh7734_dat
.ecsr_value = ECSR_ICD | ECSR_MPD,
.ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003f07ff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003f07ff,
.tx_check = EESR_TC1 | EESR_FTC,
.eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
@@ -830,7 +830,7 @@ static struct sh_eth_cpu_data sh7763_dat
.ecsr_value = ECSR_ICD | ECSR_MPD,
.ecsipr_value = ECSIPR_LCHNGIP | ECSIPR_ICDIP | ECSIPR_MPDIP,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003f07ff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003f07ff,
.tx_check = EESR_TC1 | EESR_FTC,
.eesr_err_check = EESR_TWB1 | EESR_TWB | EESR_TABT | EESR_RABT |
@@ -851,7 +851,7 @@ static struct sh_eth_cpu_data sh7763_dat
static struct sh_eth_cpu_data sh7619_data = {
.register_type = SH_ETH_REG_FAST_SH3_SH2,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003fffff,
.apr = 1,
.mpr = 1,
@@ -862,7 +862,7 @@ static struct sh_eth_cpu_data sh7619_dat
static struct sh_eth_cpu_data sh771x_data = {
.register_type = SH_ETH_REG_FAST_SH3_SH2,
- .eesipr_value = DMAC_M_RFRMER | DMAC_M_ECI | 0x003fffff,
+ .eesipr_value = EESIPR_RFCOFIP | EESIPR_ECIIP | 0x003fffff,
.tsu = 1,
};
@@ -1547,10 +1547,10 @@ static void sh_eth_emac_interrupt(struct
sh_eth_rcv_snd_disable(ndev);
} else {
/* Link Up */
- sh_eth_modify(ndev, EESIPR, DMAC_M_ECI, 0);
+ sh_eth_modify(ndev, EESIPR, EESIPR_ECIIP, 0);
/* clear int */
sh_eth_modify(ndev, ECSR, 0, 0);
- sh_eth_modify(ndev, EESIPR, DMAC_M_ECI, DMAC_M_ECI);
+ sh_eth_modify(ndev, EESIPR, EESIPR_ECIIP, EESIPR_ECIIP);
/* enable tx and rx */
sh_eth_rcv_snd_enable(ndev);
}
@@ -1652,7 +1652,7 @@ static irqreturn_t sh_eth_interrupt(int
* bit...
*/
intr_enable = sh_eth_read(ndev, EESIPR);
- intr_status &= intr_enable | DMAC_M_ECI;
+ intr_status &= intr_enable | EESIPR_ECIIP;
if (intr_status & (EESR_RX_CHECK | cd->tx_check | EESR_ECI |
cd->eesr_err_check))
ret = IRQ_HANDLED;
@@ -3199,7 +3199,7 @@ static int sh_eth_wol_setup(struct net_d
/* Only allow ECI interrupts */
synchronize_irq(ndev->irq);
napi_disable(&mdp->napi);
- sh_eth_write(ndev, DMAC_M_ECI, EESIPR);
+ sh_eth_write(ndev, EESIPR_ECIIP, EESIPR);
/* Enable MagicPacket */
sh_eth_modify(ndev, ECMR, 0, ECMR_MPDE);
Index: net-next/drivers/net/ethernet/renesas/sh_eth.h
===================================================================
--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.h
+++ net-next/drivers/net/ethernet/renesas/sh_eth.h
@@ -268,19 +268,29 @@ enum EESR_BIT {
EESR_TFE | EESR_TDE)
/* EESIPR */
-enum DMAC_IM_BIT {
- DMAC_M_TWB = 0x40000000, DMAC_M_TABT = 0x04000000,
- DMAC_M_RABT = 0x02000000,
- DMAC_M_RFRMER = 0x01000000, DMAC_M_ADF = 0x00800000,
- DMAC_M_ECI = 0x00400000, DMAC_M_FTC = 0x00200000,
- DMAC_M_TDE = 0x00100000, DMAC_M_TFE = 0x00080000,
- DMAC_M_FRC = 0x00040000, DMAC_M_RDE = 0x00020000,
- DMAC_M_RFE = 0x00010000, DMAC_M_TINT4 = 0x00000800,
- DMAC_M_TINT3 = 0x00000400, DMAC_M_TINT2 = 0x00000200,
- DMAC_M_TINT1 = 0x00000100, DMAC_M_RINT8 = 0x00000080,
- DMAC_M_RINT5 = 0x00000010, DMAC_M_RINT4 = 0x00000008,
- DMAC_M_RINT3 = 0x00000004, DMAC_M_RINT2 = 0x00000002,
- DMAC_M_RINT1 = 0x00000001,
+enum EESIPR_BIT {
+ EESIPR_TWBIP = 0x40000000,
+ EESIPR_TABTIP = 0x04000000,
+ EESIPR_RABTIP = 0x02000000,
+ EESIPR_RFCOFIP = 0x01000000,
+ EESIPR_ADEIP = 0x00800000,
+ EESIPR_ECIIP = 0x00400000,
+ EESIPR_FTCIP = 0x00200000,
+ EESIPR_TDEIP = 0x00100000,
+ EESIPR_TFUFIP = 0x00080000,
+ EESIPR_FRIP = 0x00040000,
+ EESIPR_RDEIP = 0x00020000,
+ EESIPR_RFOFIP = 0x00010000,
+ EESIPR_CNDIP = 0x00000800,
+ EESIPR_DLCIP = 0x00000400,
+ EESIPR_CDIP = 0x00000200,
+ EESIPR_TROIP = 0x00000100,
+ EESIPR_RMAFIP = 0x00000080,
+ EESIPR_RRFIP = 0x00000010,
+ EESIPR_RTLFIP = 0x00000008,
+ EESIPR_RTSFIP = 0x00000004,
+ EESIPR_PREIP = 0x00000002,
+ EESIPR_CERFIP = 0x00000001,
};
/* Receive descriptor 0 bits */
^ permalink raw reply
* [PATCH 2/3] sh_eth: add missing EESIPR bits
From: Sergei Shtylyov @ 2017-01-22 19:18 UTC (permalink / raw)
To: netdev; +Cc: linux-renesas-soc
In-Reply-To: <1636105.gp4aQxGd5K@wasted.cogentembedded.com>
Renesas SH77{34|63} manuals describe more EESIPR bits than the current
driver. Declare the new bits with the end goal of using the bit names
instead of the bare numbers for the 'sh_eth_cpu_data::eesipr_value'
initializers...
Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
---
drivers/net/ethernet/renesas/sh_eth.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Index: net-next/drivers/net/ethernet/renesas/sh_eth.h
===================================================================
--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.h
+++ net-next/drivers/net/ethernet/renesas/sh_eth.h
@@ -269,13 +269,17 @@ enum EESR_BIT {
/* EESIPR */
enum EESIPR_BIT {
- EESIPR_TWBIP = 0x40000000,
+ EESIPR_TWB1IP = 0x80000000,
+ EESIPR_TWBIP = 0x40000000, /* same as TWB0IP */
+ EESIPR_TC1IP = 0x20000000,
+ EESIPR_TUCIP = 0x10000000,
+ EESIPR_ROCIP = 0x08000000,
EESIPR_TABTIP = 0x04000000,
EESIPR_RABTIP = 0x02000000,
EESIPR_RFCOFIP = 0x01000000,
EESIPR_ADEIP = 0x00800000,
EESIPR_ECIIP = 0x00400000,
- EESIPR_FTCIP = 0x00200000,
+ EESIPR_FTCIP = 0x00200000, /* same as TC0IP */
EESIPR_TDEIP = 0x00100000,
EESIPR_TFUFIP = 0x00080000,
EESIPR_FRIP = 0x00040000,
@@ -286,6 +290,8 @@ enum EESIPR_BIT {
EESIPR_CDIP = 0x00000200,
EESIPR_TROIP = 0x00000100,
EESIPR_RMAFIP = 0x00000080,
+ EESIPR_CEEFIP = 0x00000040,
+ EESIPR_CELFIP = 0x00000020,
EESIPR_RRFIP = 0x00000010,
EESIPR_RTLFIP = 0x00000008,
EESIPR_RTSFIP = 0x00000004,
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox