* Re: [patch net-next v2 8/9] switchdev: introduce Netlink API
From: Jiri Pirko @ 2014-09-20 8:09 UTC (permalink / raw)
To: Roopa Prabhu
Cc: Jamal Hadi Salim, netdev, davem, nhorman, andy, tgraf, dborkman,
ogerlitz, jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher,
vyasevic, xiyou.wangcong, john.r.fastabend, edumazet, sfeldma,
f.fainelli, linville, dev, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck,
Shrijeet Mukherjee
In-Reply-To: <541CF75C.9030209@cumulusnetworks.com>
Sat, Sep 20, 2014 at 05:41:16AM CEST, roopa@cumulusnetworks.com wrote:
>On 9/19/14, 8:49 AM, Jiri Pirko wrote:
>>Fri, Sep 19, 2014 at 05:25:48PM CEST, jhs@mojatatu.com wrote:
>>>On 09/19/14 09:49, Jiri Pirko wrote:
>>>>This patch exposes switchdev API using generic Netlink.
>>>>Example userspace utility is here:
>>>>https://github.com/jpirko/switchdev
>>>>
>>>Is this just a temporary test tool? Otherwise i dont see reason
>>>for its existence (or the API that it feeds on).
>>Please read the conversation I had with Pravin and Jesse in v1 thread.
>>Long story short they like to have the api separated from ovs datapath
>>so ovs daemon can use it to directly communicate with driver. Also John
>>Fastabend requested a way to work with driver flows without using ovs ->
>>that was the original reason I created switchdev genl api.
>>
>>Regarding the "sw" tool, yes it is for testing purposes now. ovs daemon
>>will use directly switchdev genl api.
>>
>>I hope I cleared this out.
>We already have all the needed rtnetlink kernel api and userspace tools
>around it to support all
>switching asic features. ie, the rtnetlink api is the switchdev api. We can
>do l2, l3, acl's with it.
>Its unclear to me why we need another new netlink api. Which will mean none
>of the existing tools to
>create bridges etc will work on a switchdev.
No one is proposing such API. Note that what I'm trying to solve in my
patchset is FLOW world. There is only one API there, ovs genl. But the
usage of that for hw offload purposes was nacked by ovs maintainer. Plus
couple of people wanted to run the offloading independently on ovs
instance. Therefore I introduced the switchdev genl, which takes care of
that. No plan to extend it for other things you mentioned, just flows.
>Which seems like going in the direction exactly opposite to what we had
>discussed earlier.
Nope. The previous discussion ignored flows.
>
>If a non-ovs flow interface is needed from userspace, we can extend the
>existing interface to include flows.
How? You mean to extend rtnetlink? What advantage it would bring
comparing to separate genl iface?
>I don't understand why we should replace the existing rtnetlink switchdev api
>to accommodate flows.
Sorry, I do not undertand what "existing rtnetlink switchdev api" you
have on mind. Would you care to explain?
>
>Thanks,
>Roopa
>
^ permalink raw reply
* Re: [patch net-next v2 5/9] net: introduce dummy switch
From: Jiri Pirko @ 2014-09-20 7:37 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma, roopa,
linville, dev, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck
In-Reply-To: <541D0EBC.6010103@gmail.com>
Sat, Sep 20, 2014 at 07:21:00AM CEST, f.fainelli@gmail.com wrote:
>On 09/19/14 06:49, Jiri Pirko wrote:
>>Dummy switch implementation using switchdev interface
>
>This really looks like a DSA driver that has 0 ports, and is not attached to
>an useful network interface, and which is registering its own set of rtnl
>operations for a purpose that is unclear to me.
>
>I think registering these rtnl ops is misleading as it leads to a false idea
>that this is allowed, and that people are actually encouraged to do that for
>custom switch drivers, and this completely defeats the purpose of coming up
>with a generic API.
>
>If we are to go that route anyway, I really prefer the way Felix did it in
>swconfig, and the fake switch driver did do something useful being attached
>to the loopback interface:
>
>http://lists.openwall.net/netdev/2013/10/22/103
>http://patchwork.ozlabs.org/patch/285478/
I will drop dummyswitch because it serves primary as an example. But
since rocker is here, this is no longer needed.
>
>>
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>---
>> drivers/net/Kconfig | 7 +++
>> drivers/net/Makefile | 1 +
>> drivers/net/dummyswitch.c | 130 +++++++++++++++++++++++++++++++++++++++++++
>> include/uapi/linux/if_link.h | 9 +++
>> 4 files changed, 147 insertions(+)
>> create mode 100644 drivers/net/dummyswitch.c
>>
>>diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>>index c6f6f69..7822c74 100644
>>--- a/drivers/net/Kconfig
>>+++ b/drivers/net/Kconfig
>>@@ -71,6 +71,13 @@ config DUMMY
>> To compile this driver as a module, choose M here: the module
>> will be called dummy.
>>
>>+config NET_DUMMY_SWITCH
>>+ tristate "Dummy switch net driver support"
>>+ depends on NET_SWITCHDEV
>>+ ---help---
>>+ To compile this driver as a module, choose M here: the module
>>+ will be called dummyswitch.
>>+
>> config EQUALIZER
>> tristate "EQL (serial line load balancing) support"
>> ---help---
>>diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>>index 61aefdd..3c835ba 100644
>>--- a/drivers/net/Makefile
>>+++ b/drivers/net/Makefile
>>@@ -7,6 +7,7 @@
>> #
>> obj-$(CONFIG_BONDING) += bonding/
>> obj-$(CONFIG_DUMMY) += dummy.o
>>+obj-$(CONFIG_NET_DUMMY_SWITCH) += dummyswitch.o
>> obj-$(CONFIG_EQUALIZER) += eql.o
>> obj-$(CONFIG_IFB) += ifb.o
>> obj-$(CONFIG_MACVLAN) += macvlan.o
>>diff --git a/drivers/net/dummyswitch.c b/drivers/net/dummyswitch.c
>>new file mode 100644
>>index 0000000..e7a48f4
>>--- /dev/null
>>+++ b/drivers/net/dummyswitch.c
>>@@ -0,0 +1,130 @@
>>+/*
>>+ * drivers/net/dummyswitch.c - Dummy switch device
>>+ * Copyright (c) 2014 Jiri Pirko <jiri@resnulli.us>
>>+ *
>>+ * This program is free software; you can redistribute it and/or modify
>>+ * it under the terms of the GNU General Public License as published by
>>+ * the Free Software Foundation; either version 2 of the License, or
>>+ * (at your option) any later version.
>>+ */
>>+
>>+#include <linux/module.h>
>>+#include <linux/kernel.h>
>>+#include <linux/init.h>
>>+#include <linux/netdevice.h>
>>+#include <linux/etherdevice.h>
>>+#include <net/rtnetlink.h>
>>+
>>+struct dummyswport_priv {
>>+ struct netdev_phys_item_id psid;
>>+};
>>+
>>+static netdev_tx_t dummyswport_start_xmit(struct sk_buff *skb,
>>+ struct net_device *dev)
>>+{
>>+ dev_kfree_skb(skb);
>>+ return NETDEV_TX_OK;
>>+}
>>+
>>+static int dummyswport_swdev_id_get(struct net_device *dev,
>>+ struct netdev_phys_item_id *psid)
>>+{
>>+ struct dummyswport_priv *dsp = netdev_priv(dev);
>>+
>>+ memcpy(psid, &dsp->psid, sizeof(*psid));
>>+ return 0;
>>+}
>>+
>>+static int dummyswport_change_carrier(struct net_device *dev, bool new_carrier)
>>+{
>>+ if (new_carrier)
>>+ netif_carrier_on(dev);
>>+ else
>>+ netif_carrier_off(dev);
>>+ return 0;
>>+}
>>+
>>+static const struct net_device_ops dummyswport_netdev_ops = {
>>+ .ndo_start_xmit = dummyswport_start_xmit,
>>+ .ndo_swdev_id_get = dummyswport_swdev_id_get,
>>+ .ndo_change_carrier = dummyswport_change_carrier,
>>+};
>>+
>>+static void dummyswport_setup(struct net_device *dev)
>>+{
>>+ ether_setup(dev);
>>+
>>+ /* Initialize the device structure. */
>>+ dev->netdev_ops = &dummyswport_netdev_ops;
>>+ dev->destructor = free_netdev;
>>+
>>+ /* Fill in device structure with ethernet-generic values. */
>>+ dev->tx_queue_len = 0;
>>+ dev->flags |= IFF_NOARP;
>>+ dev->flags &= ~IFF_MULTICAST;
>>+ dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>>+ dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_TSO;
>>+ dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX;
>>+ eth_hw_addr_random(dev);
>>+}
>>+
>>+static int dummyswport_validate(struct nlattr *tb[], struct nlattr *data[])
>>+{
>>+ if (tb[IFLA_ADDRESS])
>>+ return -EINVAL;
>>+ if (!data || !data[IFLA_DUMMYSWPORT_PHYS_SWITCH_ID])
>>+ return -EINVAL;
>>+ return 0;
>>+}
>>+
>>+static int dummyswport_newlink(struct net *src_net, struct net_device *dev,
>>+ struct nlattr *tb[], struct nlattr *data[])
>>+{
>>+ struct dummyswport_priv *dsp = netdev_priv(dev);
>>+ int err;
>>+
>>+ dsp->psid.id_len = nla_len(data[IFLA_DUMMYSWPORT_PHYS_SWITCH_ID]);
>>+ memcpy(dsp->psid.id, nla_data(data[IFLA_DUMMYSWPORT_PHYS_SWITCH_ID]),
>>+ dsp->psid.id_len);
>>+
>>+ err = register_netdevice(dev);
>>+ if (err)
>>+ return err;
>>+
>>+ netif_carrier_on(dev);
>>+
>>+ return 0;
>>+}
>>+
>>+static const struct nla_policy dummyswport_policy[IFLA_DUMMYSWPORT_MAX + 1] = {
>>+ [IFLA_DUMMYSWPORT_PHYS_SWITCH_ID] = { .type = NLA_BINARY,
>>+ .len = MAX_PHYS_ITEM_ID_LEN },
>>+};
>>+
>>+static struct rtnl_link_ops dummyswport_link_ops __read_mostly = {
>>+ .kind = "dummyswport",
>>+ .priv_size = sizeof(struct dummyswport_priv),
>>+ .setup = dummyswport_setup,
>>+ .validate = dummyswport_validate,
>>+ .newlink = dummyswport_newlink,
>>+ .policy = dummyswport_policy,
>>+ .maxtype = IFLA_DUMMYSWPORT_MAX,
>>+};
>>+
>>+static int __init dummysw_module_init(void)
>>+{
>>+ return rtnl_link_register(&dummyswport_link_ops);
>>+}
>>+
>>+static void __exit dummysw_module_exit(void)
>>+{
>>+ rtnl_link_unregister(&dummyswport_link_ops);
>>+}
>>+
>>+module_init(dummysw_module_init);
>>+module_exit(dummysw_module_exit);
>>+
>>+MODULE_LICENSE("GPL v2");
>>+MODULE_AUTHOR("Jiri Pirko <jiri@resnulli.us>");
>>+MODULE_DESCRIPTION("Dummy switch device");
>>+MODULE_ALIAS_RTNL_LINK("dummyswport");
>>diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>index c5ca3b9..bd24d69 100644
>>--- a/include/uapi/linux/if_link.h
>>+++ b/include/uapi/linux/if_link.h
>>@@ -574,4 +574,13 @@ enum {
>>
>> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>>
>>+/* DUMMYSWPORT section */
>>+enum {
>>+ IFLA_DUMMYSWPORT_UNSPEC,
>>+ IFLA_DUMMYSWPORT_PHYS_SWITCH_ID,
>>+ __IFLA_DUMMYSWPORT_MAX,
>>+};
>>+
>>+#define IFLA_DUMMYSWPORT_MAX (__IFLA_DUMMYSWPORT_MAX - 1)
>>+
>> #endif /* _UAPI_LINUX_IF_LINK_H */
>>
>
^ permalink raw reply
* Re: [patch net-next v2 6/9] switchdev: add basic support for flow matching and actions
From: Jiri Pirko @ 2014-09-20 7:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, john.r.fastabend, edumazet, jhs, sfeldma, roopa,
linville, dev, jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a,
buytenh, aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye,
simon.horman, alexander.h.duyck
In-Reply-To: <541D1158.2070909@gmail.com>
Sat, Sep 20, 2014 at 07:32:08AM CEST, f.fainelli@gmail.com wrote:
>On 09/19/14 06:49, Jiri Pirko wrote:
>>This patch adds basic support for flows. The infrastructure is prepared
>>to easily add another flow matching types. So far, only the key one is
>>implemented.
>>
>>Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>>---
>
>[snip]
>
>>
>>+struct swdev_flow_match_key {
>>+ struct {
>>+ u32 priority; /* Packet QoS priority. */
>>+ u32 in_port_ifindex; /* Input switch port ifindex (or 0). */
>>+ } phy;
>>+ struct {
>>+ u8 src[ETH_ALEN]; /* Ethernet source address. */
>>+ u8 dst[ETH_ALEN]; /* Ethernet destination address. */
>>+ __be16 tci; /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
>
>Humm, how about QinQ here? I would provision two more 16 bits fields so we
>can do all sorts of VLAN matching.
>
>You might want to allow for a 4 to 8 bytes hardware switch tag as well.
Note this structure is not carved in stone and can be easily adjusted
without any problems any time. So when the time comes and the changes
you are describing will be needed, we can do it.
>
>>+ __be16 type; /* Ethernet frame type. */
>>+ } eth;
>>+ struct {
>>+ u8 proto; /* IP protocol or lower 8 bits of ARP opcode. */
>>+ u8 tos; /* IP ToS. */
>>+ u8 ttl; /* IP TTL/hop limit. */
>>+ u8 frag; /* One of OVS_FRAG_TYPE_*. */
>
>Options might be missing?
>
>[snip]
>
>>+
>>+static void print_flow(const struct swdev_flow *flow, struct net_device *dev,
>>+ const char *comment)
>>+{
>>+ pr_debug("%s flow %s:\n", dev->name, comment);
>>+ print_flow_match(&flow->match);
>>+ print_flow_actions(flow->action, flow->action_count);
>>+}
>
>I am really not sure how much of this valuable besides early (as in, right
>now) debugging, don't we rather want a generic way to dump a given flow under
>a its native netlink format, does that code has to be here in the first
>place?
Hmm, I think you have a point here, let me think about that.
>--
>Florian
^ permalink raw reply
* Re: [patch net-next v2 8/9] switchdev: introduce Netlink API
From: Florian Fainelli @ 2014-09-20 5:39 UTC (permalink / raw)
To: Jamal Hadi Salim, John Fastabend, Jiri Pirko
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, edumazet, sfeldma, roopa, linville, dev, jasowang,
ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh, aviadr, nbd,
alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck
In-Reply-To: <541CAB9A.3040100@mojatatu.com>
On 09/19/14 15:18, Jamal Hadi Salim wrote:
> On 09/19/14 18:12, John Fastabend wrote:
>> On 09/19/2014 10:57 AM, Jamal Hadi Salim wrote:
>>> On 09/19/14 11:49, Jiri Pirko wrote:
>>>> Fri, Sep 19, 2014 at 05:25:48PM CEST, jhs@mojatatu.com wrote:
>>>
>>>>> Is this just a temporary test tool? Otherwise i dont see reason
>>>>> for its existence (or the API that it feeds on).
>>>>
>>>> Please read the conversation I had with Pravin and Jesse in v1 thread.
>>>> Long story short they like to have the api separated from ovs datapath
>>>> so ovs daemon can use it to directly communicate with driver. Also John
>>>> Fastabend requested a way to work with driver flows without using
>>>> ovs ->
>>>> that was the original reason I created switchdev genl api.
>>>>
>>>> Regarding the "sw" tool, yes it is for testing purposes now. ovs daemon
>>>> will use directly switchdev genl api.
>>>>
>>>> I hope I cleared this out.
>>>>
>>>
>>> It is - thanks Jiri.
>>>
>>> cheers,
>>> jamal
>>
>> Hi Jiri,
>>
>> I was considering a slightly different approach where the
>> device would report via netlink the fields/actions it
>> supported rather than creating pre-defined enums for every
>> possible key.
>>
>> I already need to have an API to report fields/matches
>> that are being supported why not have the device report
>> the headers as header fields (len, offset) and the
>> associated parse graph the hardware uses? Vendors should
>> have this already to describe/design their real hardware.
>>
>> As always its better to have code and when I get some
>> time I'll try to write it up. Maybe its just a separate
>> classifier although I don't actually want two hardware
>> flow APIs.
>>
>> I see you dropped the RFC tag are you proposing we include
>> this now?
>>
>
>
> Actually I just realized i missed something very basic that
> Jiri said. I think i understand the tool being there for testing
> but i am assumed the same about the genlink api.
> Jiri, are you saying that genlink api is there to
> stay?
So, I really have mixed feelings about this netlink API, in particular
because it is not clear to me where is the line between what should be a
network device ndo operation, what should be an ethtool command, what
should be a netlink message, and the rest.
I can certainly acknowledge the fact that manipulating flows is not
ideal with the current set of tools, but really once we are there with
netlink, how far are we from not having any network devices at all, and
how does that differ from OpenWrt's swconfig in the end [1]?
[1]: https://lwn.net/Articles/571390/
--
Florian
^ permalink raw reply
* Re: [patch net-next v2 0/9] introduce rocker switch driver with hardware accelerated datapath api
From: Florian Fainelli @ 2014-09-20 5:37 UTC (permalink / raw)
To: Jiri Pirko, David Laight
Cc: netdev@vger.kernel.org, davem@davemloft.net,
nhorman@tuxdriver.com, andy@greyhouse.net, tgraf@suug.ch,
dborkman@redhat.com, ogerlitz@mellanox.com, jesse@nicira.com,
pshelar@nicira.com, azhou@nicira.com, ben@decadent.org.uk,
stephen@networkplumber.org, jeffrey.t.kirsher@intel.com,
vyasevic@redhat.com, xiyou.wangcong@gmail.com,
john.r.fastabend@intel.com, edumazet@google.com, jhs@mojatatu.com,
"sfeldma@cumulusnetworks.com" <sfe
In-Reply-To: <20140919142032.GF1980@nanopsycho.orion>
On 09/19/14 07:20, Jiri Pirko wrote:
> Fri, Sep 19, 2014 at 04:15:32PM CEST, David.Laight@ACULAB.COM wrote:
>> From: Jiri Pirko
>>> This patchset can be divided into 3 main sections:
>>> - introduce switchdev api for implementing switch drivers
>>> - introduce switchdev generic netlink api for userspace manipulation
>>> - introduce rocker switch driver which implements switchdev api
>>
>> Perhaps you should be including the name of what you are switching
>> in the name of the API?
>>
>> Is this for interfacing to ethernet switches, TDM switches or
>> mechanical ones?
>> It isn't really clear from any of these commit messages.
>
> I thought that is isn't necessary, that it is clear this is about ethernet
> switches.
>
How about putting some of this code in net/ethernet/* to make very very
obvious this is what this thing is about?
--
Florian
^ permalink raw reply
* Re: [patch net-next v2 8/9] switchdev: introduce Netlink API
From: Florian Fainelli @ 2014-09-20 5:36 UTC (permalink / raw)
To: John Fastabend, Jamal Hadi Salim, Jiri Pirko
Cc: netdev, davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse,
pshelar, azhou, ben, stephen, jeffrey.t.kirsher, vyasevic,
xiyou.wangcong, edumazet, sfeldma, roopa, linville, dev, jasowang,
ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh, aviadr, nbd,
alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck
In-Reply-To: <541CAA3C.5080105@intel.com>
On 09/19/14 15:12, John Fastabend wrote:
> On 09/19/2014 10:57 AM, Jamal Hadi Salim wrote:
>> On 09/19/14 11:49, Jiri Pirko wrote:
>>> Fri, Sep 19, 2014 at 05:25:48PM CEST, jhs@mojatatu.com wrote:
>>
>>>> Is this just a temporary test tool? Otherwise i dont see reason
>>>> for its existence (or the API that it feeds on).
>>>
>>> Please read the conversation I had with Pravin and Jesse in v1 thread.
>>> Long story short they like to have the api separated from ovs datapath
>>> so ovs daemon can use it to directly communicate with driver. Also John
>>> Fastabend requested a way to work with driver flows without using ovs ->
>>> that was the original reason I created switchdev genl api.
>>>
>>> Regarding the "sw" tool, yes it is for testing purposes now. ovs daemon
>>> will use directly switchdev genl api.
>>>
>>> I hope I cleared this out.
>>>
>>
>> It is - thanks Jiri.
>>
>> cheers,
>> jamal
>
> Hi Jiri,
>
> I was considering a slightly different approach where the
> device would report via netlink the fields/actions it
> supported rather than creating pre-defined enums for every
> possible key.
>
> I already need to have an API to report fields/matches
> that are being supported why not have the device report
> the headers as header fields (len, offset) and the
> associated parse graph the hardware uses? Vendors should
> have this already to describe/design their real hardware.
Humm would not that slightly go against coming with a netlink API that
is generic? Surely we could pay close attention when reviewing what is
being added and spot when a common API needs to be introduced...
This might become very similar to the private ioctl(), private wireless
extensions, nl80211 testmode and well it's not extremely pretty.
>
> As always its better to have code and when I get some
> time I'll try to write it up. Maybe its just a separate
> classifier although I don't actually want two hardware
> flow APIs.
>
> I see you dropped the RFC tag are you proposing we include
> this now?
>
> .John
>
^ permalink raw reply
* Re: [patch net-next v2 6/9] switchdev: add basic support for flow matching and actions
From: Florian Fainelli @ 2014-09-20 5:32 UTC (permalink / raw)
To: Jiri Pirko, netdev
Cc: davem, nhorman, andy, tgraf, dborkman, ogerlitz, jesse, pshelar,
azhou, ben, stephen, jeffrey.t.kirsher, vyasevic, xiyou.wangcong,
john.r.fastabend, edumazet, jhs, sfeldma, roopa, linville, dev,
jasowang, ebiederm, nicolas.dichtel, ryazanov.s.a, buytenh,
aviadr, nbd, alexei.starovoitov, Neil.Jerram, ronye, simon.horman,
alexander.h.duyck
In-Reply-To: <1411134590-4586-7-git-send-email-jiri@resnulli.us>
On 09/19/14 06:49, Jiri Pirko wrote:
> This patch adds basic support for flows. The infrastructure is prepared
> to easily add another flow matching types. So far, only the key one is
> implemented.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
[snip]
>
> +struct swdev_flow_match_key {
> + struct {
> + u32 priority; /* Packet QoS priority. */
> + u32 in_port_ifindex; /* Input switch port ifindex (or 0). */
> + } phy;
> + struct {
> + u8 src[ETH_ALEN]; /* Ethernet source address. */
> + u8 dst[ETH_ALEN]; /* Ethernet destination address. */
> + __be16 tci; /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
Humm, how about QinQ here? I would provision two more 16 bits fields so
we can do all sorts of VLAN matching.
You might want to allow for a 4 to 8 bytes hardware switch tag as well.
> + __be16 type; /* Ethernet frame type. */
> + } eth;
> + struct {
> + u8 proto; /* IP protocol or lower 8 bits of ARP opcode. */
> + u8 tos; /* IP ToS. */
> + u8 ttl; /* IP TTL/hop limit. */
> + u8 frag; /* One of OVS_FRAG_TYPE_*. */
Options might be missing?
[snip]
> +
> +static void print_flow(const struct swdev_flow *flow, struct net_device *dev,
> + const char *comment)
> +{
> + pr_debug("%s flow %s:\n", dev->name, comment);
> + print_flow_match(&flow->match);
> + print_flow_actions(flow->action, flow->action_count);
> +}
I am really not sure how much of this valuable besides early (as in,
right now) debugging, don't we rather want a generic way to dump a given
flow under a its native netlink format, does that code has to be here in
the first place?
--
Florian
^ permalink raw reply
* Re: [patch net-next v2 5/9] net: introduce dummy switch
From: Florian Fainelli @ 2014-09-20 5:21 UTC (permalink / raw)
To: Jiri Pirko, netdev-u79uwXL29TY76Z2rM5mHXA
Cc: ryazanov.s.a-Re5JQEeQqe8AvxtiuMwx3w,
jasowang-H+wXaHxf7aLQT0dZR+AlfA,
john.r.fastabend-ral2JQCrhuEAvxtiuMwx3w,
Neil.Jerram-QnUH15yq9NYqDJ6do+/SaQ,
edumazet-hpIqsD4AKlfQT0dZR+AlfA, andy-QlMahl40kYEqcZcGjlUOXw,
dev-yBygre7rU0TnMu66kgdUjQ, nbd-p3rKhJxN3npAfugRpC6u6w,
ronye-VPRAkNaXOzVWk0Htik3J/w,
jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w,
ogerlitz-VPRAkNaXOzVWk0Htik3J/w, ben-/+tVBieCtBitmTQ+vhA3Yw,
buytenh-OLH4Qvv75CYX/NnBR394Jw,
alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w,
simon.horman-wFxRvT7yatFl57MIdRCFDg,
roopa-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR,
jhs-jkUAjuhPggJWk0Htik3J/w, aviadr-VPRAkNaXOzVWk0Htik3J/w,
nicolas.dichtel-pdR9zngts4EAvxtiuMwx3w,
vyasevic-H+wXaHxf7aLQT0dZR+AlfA, nhorman-2XuSBdqkA4R54TAoqtyWWQ,
stephen-OTpzqLSitTUnbdJkjeBofR2eb7JE58TQ,
dborkman-H+wXaHxf7aLQT0dZR+AlfA, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
In-Reply-To: <1411134590-4586-6-git-send-email-jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
On 09/19/14 06:49, Jiri Pirko wrote:
> Dummy switch implementation using switchdev interface
This really looks like a DSA driver that has 0 ports, and is not
attached to an useful network interface, and which is registering its
own set of rtnl operations for a purpose that is unclear to me.
I think registering these rtnl ops is misleading as it leads to a false
idea that this is allowed, and that people are actually encouraged to do
that for custom switch drivers, and this completely defeats the purpose
of coming up with a generic API.
If we are to go that route anyway, I really prefer the way Felix did it
in swconfig, and the fake switch driver did do something useful being
attached to the loopback interface:
http://lists.openwall.net/netdev/2013/10/22/103
http://patchwork.ozlabs.org/patch/285478/
>
> Signed-off-by: Jiri Pirko <jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
> ---
> drivers/net/Kconfig | 7 +++
> drivers/net/Makefile | 1 +
> drivers/net/dummyswitch.c | 130 +++++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/if_link.h | 9 +++
> 4 files changed, 147 insertions(+)
> create mode 100644 drivers/net/dummyswitch.c
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index c6f6f69..7822c74 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -71,6 +71,13 @@ config DUMMY
> To compile this driver as a module, choose M here: the module
> will be called dummy.
>
> +config NET_DUMMY_SWITCH
> + tristate "Dummy switch net driver support"
> + depends on NET_SWITCHDEV
> + ---help---
> + To compile this driver as a module, choose M here: the module
> + will be called dummyswitch.
> +
> config EQUALIZER
> tristate "EQL (serial line load balancing) support"
> ---help---
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 61aefdd..3c835ba 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -7,6 +7,7 @@
> #
> obj-$(CONFIG_BONDING) += bonding/
> obj-$(CONFIG_DUMMY) += dummy.o
> +obj-$(CONFIG_NET_DUMMY_SWITCH) += dummyswitch.o
> obj-$(CONFIG_EQUALIZER) += eql.o
> obj-$(CONFIG_IFB) += ifb.o
> obj-$(CONFIG_MACVLAN) += macvlan.o
> diff --git a/drivers/net/dummyswitch.c b/drivers/net/dummyswitch.c
> new file mode 100644
> index 0000000..e7a48f4
> --- /dev/null
> +++ b/drivers/net/dummyswitch.c
> @@ -0,0 +1,130 @@
> +/*
> + * drivers/net/dummyswitch.c - Dummy switch device
> + * Copyright (c) 2014 Jiri Pirko <jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <net/rtnetlink.h>
> +
> +struct dummyswport_priv {
> + struct netdev_phys_item_id psid;
> +};
> +
> +static netdev_tx_t dummyswport_start_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + dev_kfree_skb(skb);
> + return NETDEV_TX_OK;
> +}
> +
> +static int dummyswport_swdev_id_get(struct net_device *dev,
> + struct netdev_phys_item_id *psid)
> +{
> + struct dummyswport_priv *dsp = netdev_priv(dev);
> +
> + memcpy(psid, &dsp->psid, sizeof(*psid));
> + return 0;
> +}
> +
> +static int dummyswport_change_carrier(struct net_device *dev, bool new_carrier)
> +{
> + if (new_carrier)
> + netif_carrier_on(dev);
> + else
> + netif_carrier_off(dev);
> + return 0;
> +}
> +
> +static const struct net_device_ops dummyswport_netdev_ops = {
> + .ndo_start_xmit = dummyswport_start_xmit,
> + .ndo_swdev_id_get = dummyswport_swdev_id_get,
> + .ndo_change_carrier = dummyswport_change_carrier,
> +};
> +
> +static void dummyswport_setup(struct net_device *dev)
> +{
> + ether_setup(dev);
> +
> + /* Initialize the device structure. */
> + dev->netdev_ops = &dummyswport_netdev_ops;
> + dev->destructor = free_netdev;
> +
> + /* Fill in device structure with ethernet-generic values. */
> + dev->tx_queue_len = 0;
> + dev->flags |= IFF_NOARP;
> + dev->flags &= ~IFF_MULTICAST;
> + dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> + dev->features |= NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_TSO;
> + dev->features |= NETIF_F_HW_CSUM | NETIF_F_HIGHDMA | NETIF_F_LLTX;
> + eth_hw_addr_random(dev);
> +}
> +
> +static int dummyswport_validate(struct nlattr *tb[], struct nlattr *data[])
> +{
> + if (tb[IFLA_ADDRESS])
> + return -EINVAL;
> + if (!data || !data[IFLA_DUMMYSWPORT_PHYS_SWITCH_ID])
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int dummyswport_newlink(struct net *src_net, struct net_device *dev,
> + struct nlattr *tb[], struct nlattr *data[])
> +{
> + struct dummyswport_priv *dsp = netdev_priv(dev);
> + int err;
> +
> + dsp->psid.id_len = nla_len(data[IFLA_DUMMYSWPORT_PHYS_SWITCH_ID]);
> + memcpy(dsp->psid.id, nla_data(data[IFLA_DUMMYSWPORT_PHYS_SWITCH_ID]),
> + dsp->psid.id_len);
> +
> + err = register_netdevice(dev);
> + if (err)
> + return err;
> +
> + netif_carrier_on(dev);
> +
> + return 0;
> +}
> +
> +static const struct nla_policy dummyswport_policy[IFLA_DUMMYSWPORT_MAX + 1] = {
> + [IFLA_DUMMYSWPORT_PHYS_SWITCH_ID] = { .type = NLA_BINARY,
> + .len = MAX_PHYS_ITEM_ID_LEN },
> +};
> +
> +static struct rtnl_link_ops dummyswport_link_ops __read_mostly = {
> + .kind = "dummyswport",
> + .priv_size = sizeof(struct dummyswport_priv),
> + .setup = dummyswport_setup,
> + .validate = dummyswport_validate,
> + .newlink = dummyswport_newlink,
> + .policy = dummyswport_policy,
> + .maxtype = IFLA_DUMMYSWPORT_MAX,
> +};
> +
> +static int __init dummysw_module_init(void)
> +{
> + return rtnl_link_register(&dummyswport_link_ops);
> +}
> +
> +static void __exit dummysw_module_exit(void)
> +{
> + rtnl_link_unregister(&dummyswport_link_ops);
> +}
> +
> +module_init(dummysw_module_init);
> +module_exit(dummysw_module_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Jiri Pirko <jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org>");
> +MODULE_DESCRIPTION("Dummy switch device");
> +MODULE_ALIAS_RTNL_LINK("dummyswport");
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index c5ca3b9..bd24d69 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -574,4 +574,13 @@ enum {
>
> #define IFLA_HSR_MAX (__IFLA_HSR_MAX - 1)
>
> +/* DUMMYSWPORT section */
> +enum {
> + IFLA_DUMMYSWPORT_UNSPEC,
> + IFLA_DUMMYSWPORT_PHYS_SWITCH_ID,
> + __IFLA_DUMMYSWPORT_MAX,
> +};
> +
> +#define IFLA_DUMMYSWPORT_MAX (__IFLA_DUMMYSWPORT_MAX - 1)
> +
> #endif /* _UAPI_LINUX_IF_LINK_H */
>
^ permalink raw reply
* Re: [net-next PATCH 2/2] net: sched: cls_u32 changes to knode must appear atomic to readers
From: John Fastabend @ 2014-09-20 4:55 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: xiyou.wangcong, davem, eric.dumazet, netdev
In-Reply-To: <541AC425.2090300@mojatatu.com>
On 09/18/2014 04:38 AM, Jamal Hadi Salim wrote:
> On 09/17/14 15:12, John Fastabend wrote:
>> Changes to the cls_u32 classifier must appear atomic to the
>> readers. Before this patch if a change is requested for both
>> the exts and ifindex, first the ifindex is updated then the
>> exts with tcf_exts_change(). This opens a small window where
>> a reader can have a exts chain with an incorrect ifindex. This
>> violates the the RCU semantics.
>>
>> Here we resolve this by always passing u32_set_parms() a copy
>> of the tc_u_knode to work on and then inserting it into the hash
>> table after the updates have been successfully applied.
>>
>> Tested with the following short script:
>>
>
>>
>> #tc filter add dev p3p2 parent 8001:0 protocol ip prio 99 handle 1: \
>> u32 divisor 256
>>
>> #tc filter add dev p3p2 parent 8001:0 protocol ip prio 99 \
>> u32 link 1: hashkey mask ffffff00 at 12 \
>> match ip src 192.168.8.0/2
>>
>> #tc filter add dev p3p2 parent 8001:0 protocol ip prio 102 \
>> handle 1::10 u32 classid 1:2 ht 1: \
>> match ip src 192.168.8.0/8 match ip tos 0x0a 1e
>>
>> #tc filter change dev p3p2 parent 8001:0 protocol ip prio 102 \
>> handle 1::10 u32 classid 1:2 ht 1: \
>> match ip src 1.1.0.0/8 match ip tos 0x0b 1e
>>
>> CC: Eric Dumazet <edumazet@google.com>
>> CC: Jamal Hadi Salim <jhs@mojatatu.com>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>
>
> Looks good to me.
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
> cheers,
> jamal
Thanks for looking it over! I made v2 though to address a comment
that my variable/function names could be better and added a comment
around the perhaps tricky cases where it is safe to free the percpu
variables.
Because I did touch the patch and make some changes I dropped your
ACK. I always thought it was a bit of bad form to carry ack's around
after modifying the code without an explicit approval. Please add it
back though if you want.
Thanks,
John
--
John Fastabend Intel Corporation
^ permalink raw reply
* [net-next PATCH v2 2/2] net: sched: cls_u32 changes to knode must appear atomic to readers
From: John Fastabend @ 2014-09-20 4:50 UTC (permalink / raw)
To: xiyou.wangcong, jhs, eric.dumazet, davem; +Cc: netdev
In-Reply-To: <20140920045003.9869.16851.stgit@nitbit.x32>
Changes to the cls_u32 classifier must appear atomic to the
readers. Before this patch if a change is requested for both
the exts and ifindex, first the ifindex is updated then the
exts with tcf_exts_change(). This opens a small window where
a reader can have a exts chain with an incorrect ifindex. This
violates the the RCU semantics.
Here we resolve this by always passing u32_set_parms() a copy
of the tc_u_knode to work on and then inserting it into the hash
table after the updates have been successfully applied.
Tested with the following short script:
#tc filter add dev p3p2 parent 8001:0 protocol ip prio 99 handle 1: \
u32 divisor 256
#tc filter add dev p3p2 parent 8001:0 protocol ip prio 99 \
u32 link 1: hashkey mask ffffff00 at 12 \
match ip src 192.168.8.0/2
#tc filter add dev p3p2 parent 8001:0 protocol ip prio 102 \
handle 1::10 u32 classid 1:2 ht 1: \
match ip src 192.168.8.0/8 match ip tos 0x0a 1e
#tc filter change dev p3p2 parent 8001:0 protocol ip prio 102 \
handle 1::10 u32 classid 1:2 ht 1: \
match ip src 1.1.0.0/8 match ip tos 0x0b 1e
CC: Eric Dumazet <edumazet@google.com>
CC: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
net/sched/cls_u32.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 126 insertions(+), 9 deletions(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 8d90e50..e3fb530 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -354,27 +354,53 @@ static int u32_init(struct tcf_proto *tp)
return 0;
}
-static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n)
+static int u32_destroy_key(struct tcf_proto *tp,
+ struct tc_u_knode *n,
+ bool free_pf)
{
tcf_unbind_filter(tp, &n->res);
tcf_exts_destroy(tp, &n->exts);
if (n->ht_down)
n->ht_down->refcnt--;
#ifdef CONFIG_CLS_U32_PERF
- free_percpu(n->pf);
+ if (free_pf)
+ free_percpu(n->pf);
#endif
#ifdef CONFIG_CLS_U32_MARK
- free_percpu(n->pcpu_success);
+ if (free_pf)
+ free_percpu(n->pcpu_success);
#endif
kfree(n);
return 0;
}
+/* u32_delete_key_rcu should be called when free'ing a copied
+ * version of a tc_u_knode obtained from u32_init_knode(). When
+ * copies are obtained from u32_init_knode() the statistics are
+ * shared between the old and new copies to allow readers to
+ * continue to update the statistics during the copy. To support
+ * this the u32_delete_key_rcu variant does not free the percpu
+ * statistics.
+ */
static void u32_delete_key_rcu(struct rcu_head *rcu)
{
struct tc_u_knode *key = container_of(rcu, struct tc_u_knode, rcu);
- u32_destroy_key(key->tp, key);
+ u32_destroy_key(key->tp, key, false);
+}
+
+/* u32_delete_key_freepf_rcu is the rcu callback variant
+ * that free's the entire structure including the statistics
+ * percpu variables. Only use this if the key is not a copy
+ * returned by u32_init_knode(). See u32_delete_key_rcu()
+ * for the variant that should be used with keys return from
+ * u32_init_knode()
+ */
+static void u32_delete_key_freepf_rcu(struct rcu_head *rcu)
+{
+ struct tc_u_knode *key = container_of(rcu, struct tc_u_knode, rcu);
+
+ u32_destroy_key(key->tp, key, true);
}
static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
@@ -390,7 +416,7 @@ static int u32_delete_key(struct tcf_proto *tp, struct tc_u_knode *key)
if (pkp == key) {
RCU_INIT_POINTER(*kp, key->next);
- call_rcu(&key->rcu, u32_delete_key_rcu);
+ call_rcu(&key->rcu, u32_delete_key_freepf_rcu);
return 0;
}
}
@@ -408,7 +434,7 @@ static void u32_clear_hnode(struct tc_u_hnode *ht)
while ((n = rtnl_dereference(ht->ht[h])) != NULL) {
RCU_INIT_POINTER(ht->ht[h],
rtnl_dereference(n->next));
- call_rcu(&n->rcu, u32_delete_key_rcu);
+ call_rcu(&n->rcu, u32_delete_key_freepf_rcu);
}
}
}
@@ -584,6 +610,82 @@ errout:
return err;
}
+static void u32_replace_knode(struct tcf_proto *tp,
+ struct tc_u_common *tp_c,
+ struct tc_u_knode *n)
+{
+ struct tc_u_knode __rcu **ins;
+ struct tc_u_knode *pins;
+ struct tc_u_hnode *ht;
+
+ if (TC_U32_HTID(n->handle) == TC_U32_ROOT)
+ ht = rtnl_dereference(tp->root);
+ else
+ ht = u32_lookup_ht(tp_c, TC_U32_HTID(n->handle));
+
+ ins = &ht->ht[TC_U32_HASH(n->handle)];
+
+ /* The node must always exist for it to be replaced if this is not the
+ * case then something went very wrong elsewhere.
+ */
+ for (pins = rtnl_dereference(*ins); ;
+ ins = &pins->next, pins = rtnl_dereference(*ins))
+ if (pins->handle == n->handle)
+ break;
+
+ RCU_INIT_POINTER(n->next, pins->next);
+ rcu_assign_pointer(*ins, n);
+}
+
+static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp,
+ struct tc_u_knode *n)
+{
+ struct tc_u_knode *new;
+ struct tc_u32_sel *s = &n->sel;
+
+ new = kzalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key),
+ GFP_KERNEL);
+
+ if (!new)
+ return NULL;
+
+ RCU_INIT_POINTER(new->next, n->next);
+ new->handle = n->handle;
+ RCU_INIT_POINTER(new->ht_up, n->ht_up);
+
+#ifdef CONFIG_NET_CLS_IND
+ new->ifindex = n->ifindex;
+#endif
+ new->fshift = n->fshift;
+ new->res = n->res;
+ RCU_INIT_POINTER(new->ht_down, n->ht_down);
+
+ /* bump reference count as long as we hold pointer to structure */
+ if (new->ht_down)
+ new->ht_down->refcnt++;
+
+#ifdef CONFIG_CLS_U32_PERF
+ /* Statistics may be incremented by readers during update
+ * so we must keep them in tact. When the node is later destroyed
+ * a special destroy call must be made to not free the pf memory.
+ */
+ new->pf = n->pf;
+#endif
+
+#ifdef CONFIG_CLS_U32_MARK
+ new->val = n->val;
+ new->mask = n->mask;
+ /* Similarly success statistics must be moved as pointers */
+ new->pcpu_success = n->pcpu_success;
+#endif
+ new->tp = tp;
+ memcpy(&new->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
+
+ tcf_exts_init(&new->exts, TCA_U32_ACT, TCA_U32_POLICE);
+
+ return new;
+}
+
static int u32_change(struct net *net, struct sk_buff *in_skb,
struct tcf_proto *tp, unsigned long base, u32 handle,
struct nlattr **tca,
@@ -610,12 +712,27 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
n = (struct tc_u_knode *)*arg;
if (n) {
+ struct tc_u_knode *new;
+
if (TC_U32_KEY(n->handle) == 0)
return -EINVAL;
- return u32_set_parms(net, tp, base,
- rtnl_dereference(n->ht_up), n, tb,
- tca[TCA_RATE], ovr);
+ new = u32_init_knode(tp, n);
+ if (!new)
+ return -ENOMEM;
+
+ err = u32_set_parms(net, tp, base,
+ rtnl_dereference(n->ht_up), new, tb,
+ tca[TCA_RATE], ovr);
+
+ if (err) {
+ u32_destroy_key(tp, new, false);
+ return err;
+ }
+
+ u32_replace_knode(tp, tp_c, new);
+ call_rcu(&n->rcu, u32_delete_key_rcu);
+ return 0;
}
if (tb[TCA_U32_DIVISOR]) {
^ permalink raw reply related
* [net-next PATCH v2 1/2] net: cls_u32: fix missed pcpu_success free_percpu
From: John Fastabend @ 2014-09-20 4:50 UTC (permalink / raw)
To: xiyou.wangcong, jhs, eric.dumazet, davem; +Cc: netdev
This fixes a missed free_percpu in the unwind code path and when
keys are destroyed.
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
net/sched/cls_u32.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 730edb2..8d90e50 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -363,6 +363,9 @@ static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n)
#ifdef CONFIG_CLS_U32_PERF
free_percpu(n->pf);
#endif
+#ifdef CONFIG_CLS_U32_MARK
+ free_percpu(n->pcpu_success);
+#endif
kfree(n);
return 0;
}
@@ -693,6 +696,10 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
#ifdef CONFIG_CLS_U32_MARK
n->pcpu_success = alloc_percpu(u32);
+ if (!n->pcpu_success) {
+ err = -ENOMEM;
+ goto errout;
+ }
if (tb[TCA_U32_MARK]) {
struct tc_u32_mark *mark;
@@ -720,6 +727,12 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
*arg = (unsigned long)n;
return 0;
}
+
+#ifdef CONFIG_CLS_U32_MARK
+ free_percpu(n->pcpu_success);
+#endif
+
+errout:
#ifdef CONFIG_CLS_U32_PERF
free_percpu(n->pf);
#endif
^ permalink raw reply related
* Re: [patch net-next v2 8/9] switchdev: introduce Netlink API
From: Roopa Prabhu @ 2014-09-20 3:41 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jamal Hadi Salim, netdev, davem, nhorman, andy, tgraf, dborkman,
ogerlitz, jesse, pshelar, azhou, ben, stephen, jeffrey.t.kirsher,
vyasevic, xiyou.wangcong, john.r.fastabend, edumazet, sfeldma,
f.fainelli, linville, dev, jasowang, ebiederm, nicolas.dichtel,
ryazanov.s.a, buytenh, aviadr, nbd, alexei.starovoitov,
Neil.Jerram, ronye, simon.horman, alexander.h.duyck,
Shrijeet Mukherjee
In-Reply-To: <20140919154946.GH1980@nanopsycho.orion>
On 9/19/14, 8:49 AM, Jiri Pirko wrote:
> Fri, Sep 19, 2014 at 05:25:48PM CEST, jhs@mojatatu.com wrote:
>> On 09/19/14 09:49, Jiri Pirko wrote:
>>> This patch exposes switchdev API using generic Netlink.
>>> Example userspace utility is here:
>>> https://github.com/jpirko/switchdev
>>>
>> Is this just a temporary test tool? Otherwise i dont see reason
>> for its existence (or the API that it feeds on).
> Please read the conversation I had with Pravin and Jesse in v1 thread.
> Long story short they like to have the api separated from ovs datapath
> so ovs daemon can use it to directly communicate with driver. Also John
> Fastabend requested a way to work with driver flows without using ovs ->
> that was the original reason I created switchdev genl api.
>
> Regarding the "sw" tool, yes it is for testing purposes now. ovs daemon
> will use directly switchdev genl api.
>
> I hope I cleared this out.
We already have all the needed rtnetlink kernel api and userspace tools
around it to support all
switching asic features. ie, the rtnetlink api is the switchdev api. We
can do l2, l3, acl's with it.
Its unclear to me why we need another new netlink api. Which will mean
none of the existing tools to
create bridges etc will work on a switchdev.
Which seems like going in the direction exactly opposite to what we had
discussed earlier.
If a non-ovs flow interface is needed from userspace, we can extend the
existing interface to include flows.
I don't understand why we should replace the existing rtnetlink
switchdev api to accommodate flows.
Thanks,
Roopa
^ permalink raw reply
* Re: [net-next] udp_tunnel: Only build ip6_udp_tunnel.c when IPV6 is selected
From: David Miller @ 2014-09-20 2:06 UTC (permalink / raw)
To: azhou; +Cc: netdev
In-Reply-To: <1411174973-323-1-git-send-email-azhou@nicira.com>
From: Andy Zhou <azhou@nicira.com>
Date: Fri, 19 Sep 2014 18:02:53 -0700
> Functions supplied in ip6_udp_tunnel.c are only needed when IPV6 is
> selected. When IPV6 is not selected, those functions are stubbed out
> in udp_tunnel.h.
>
> ==================================================================
> net/ipv6/ip6_udp_tunnel.c:15:5: error: redefinition of 'udp_sock_create6'
> int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
> In file included from net/ipv6/ip6_udp_tunnel.c:9:0:
> include/net/udp_tunnel.h:36:19: note: previous definition of 'udp_sock_create6' was here
> static inline int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
> ==================================================================
>
> Fixes: fd384412e udp_tunnel: Seperate ipv6 functions into its own file
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Andy Zhou <azhou@nicira.com>
Applied, thanks Andy.
^ permalink raw reply
* Re: [net-next PATCH 00/29] Add support for the Intel FM10000 Ethernet Switch Host Interface
From: David Miller @ 2014-09-20 2:03 UTC (permalink / raw)
To: alexander.duyck
Cc: alexander.h.duyck, nhorman, netdev, john.fastabend, matthew.vick,
jeffrey.t.kirsher, sassmann
In-Reply-To: <541CC1C0.9090204@gmail.com>
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Fri, 19 Sep 2014 16:52:32 -0700
> I was wondering what changes you wanted for these patches? The only
> feedback that I have gotten so far was on the last 2 patches, and a one
> liner about dev_consume_skb_any vs dev_kfree_skb_any. Is there
> something I missed, or do I need to resubmit the entire set with some
> other sort of fix?
Any time there is any change requested or intended adjustment, I mark
the entire series as changes requested, and you are expected to make
the adjustments and resubmit the entire series.
I do this silently to work my queue as efficiently as possible, and yet
I still sometimes fall very far behind and get backlogged.
Please do not ask me to repeat what's already been stated in the patch
followup postings as I'll fall even further behind.
Thanks.
^ permalink raw reply
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
From: David Miller @ 2014-09-20 2:00 UTC (permalink / raw)
To: eric.dumazet; +Cc: cwang, nicolas.dichtel, netdev, herbert
In-Reply-To: <1411168172.26859.61.camel@edumazet-glaptop2.roam.corp.google.com>
Guys just change the check in macvlan to >= tx_queue_len or similar
and let's end this silly discussion.
^ permalink raw reply
* Re: [PATCH 1/2] 3c59x: Add dma error checking and recovery
From: Neil Horman @ 2014-09-20 1:19 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, mroos
In-Reply-To: <20140919.162919.2196253287493171915.davem@davemloft.net>
On Fri, Sep 19, 2014 at 04:29:19PM -0400, David Miller wrote:
> From: Neil Horman <nhorman@tuxdriver.com>
> Date: Wed, 17 Sep 2014 09:04:44 -0400
>
> > Noted that 3c59x has no checks on transmit for failed DMA mappings, and no
> > ability to unmap fragments when a single map fails in the middle of a transmit.
> > This patch provides error checking to ensure that dma mappings work properly,
> > and unrolls an skb mapping if a fragmented skb transmission has a mapping
> > failure to prevent leaks.
> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Linux Kernel list <linux-kernel@vger.kernel.org>
> > CC: "David S. Miller" <davem@davemloft.net>
> > CC: Meelis Roos <mroos@linux.ee>
> > Tested-by: Meelis Roos <mroos@linux.ee>
>
> Applied.
>
Thanks Dave!
Neil
^ permalink raw reply
* [net-next] udp_tunnel: Only build ip6_udp_tunnel.c when IPV6 is selected
From: Andy Zhou @ 2014-09-20 1:02 UTC (permalink / raw)
To: davem; +Cc: netdev, Andy Zhou
Functions supplied in ip6_udp_tunnel.c are only needed when IPV6 is
selected. When IPV6 is not selected, those functions are stubbed out
in udp_tunnel.h.
==================================================================
net/ipv6/ip6_udp_tunnel.c:15:5: error: redefinition of 'udp_sock_create6'
int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
In file included from net/ipv6/ip6_udp_tunnel.c:9:0:
include/net/udp_tunnel.h:36:19: note: previous definition of 'udp_sock_create6' was here
static inline int udp_sock_create6(struct net *net, struct udp_port_cfg *cfg,
==================================================================
Fixes: fd384412e udp_tunnel: Seperate ipv6 functions into its own file
Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Andy Zhou <azhou@nicira.com>
---
net/ipv6/Makefile | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/Makefile b/net/ipv6/Makefile
index 45f830e..2e8c061 100644
--- a/net/ipv6/Makefile
+++ b/net/ipv6/Makefile
@@ -35,7 +35,6 @@ obj-$(CONFIG_INET6_XFRM_MODE_ROUTEOPTIMIZATION) += xfrm6_mode_ro.o
obj-$(CONFIG_INET6_XFRM_MODE_BEET) += xfrm6_mode_beet.o
obj-$(CONFIG_IPV6_MIP6) += mip6.o
obj-$(CONFIG_NETFILTER) += netfilter/
-obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o
obj-$(CONFIG_IPV6_VTI) += ip6_vti.o
obj-$(CONFIG_IPV6_SIT) += sit.o
@@ -46,3 +45,7 @@ obj-y += addrconf_core.o exthdrs_core.o ip6_checksum.o ip6_icmp.o
obj-$(CONFIG_INET) += output_core.o protocol.o $(ipv6-offload)
obj-$(subst m,y,$(CONFIG_IPV6)) += inet6_hashtables.o
+
+ifneq ($(CONFIG_IPV6),)
+obj-$(CONFIG_NET_UDP_TUNNEL) += ip6_udp_tunnel.o
+endif
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH v2 0/2] tg3: Fix handling of non-acceleration vlans
From: Prashant Sreedharan @ 2014-09-20 0:39 UTC (permalink / raw)
To: Vladislav Yasevich; +Cc: netdev, Vladislav Yasevich, Michael Chan
In-Reply-To: <1411165389-21276-1-git-send-email-vyasevic@redhat.com>
On Fri, 2014-09-19 at 18:23 -0400, Vladislav Yasevich wrote:
> TG3 can't cope with checksum and TSO offloads when vlan headers were
> not accelerated. This can be demonstrated with 802.1ad vlans or
> by configuring a vlan on top of a bridge and turning off vlan acceleration
> on the bridge device.
>
> Instead of disabling all vlan acceleration, this series works around
> the issue by having tg3 driver call software segmentation and checksum
> generation.
>
> v2: - moved the call to software segment soonner
> - Preserve the checksum flags for TSO/LSO case and turn it off
> if software checksum was computed.
> - Add code to correctly receive full sized 802.1ad frames.
>
> Vladislav Yasevich (2):
> tg3: Work around HW/FW limitations with vlan encapsulated frames
> tg3: Allow for receive of full-size 8021AD frames
>
> drivers/net/ethernet/broadcom/tg3.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)
>
Acked-by: Prashant Sreedharan <prashant@broadcom.com>
^ permalink raw reply
* Re: [RFC net-next PATCH V3 0/2] qdisc bulk dequeuing and utilizing delayed tailptr updates
From: Tom Herbert @ 2014-09-20 0:31 UTC (permalink / raw)
To: Eric Dumazet
Cc: Jesper Dangaard Brouer, Linux Netdev List, David S. Miller,
Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
Toke Høiland-Jørgensen, Tino Reichardt
In-Reply-To: <1411160360.26859.31.camel@edumazet-glaptop2.roam.corp.google.com>
On Fri, Sep 19, 2014 at 1:59 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2014-09-19 at 22:49 +0200, Jesper Dangaard Brouer wrote:
>> This patchset uses DaveM's recent API changes to dev_hard_start_xmit(),
>> from the qdisc layer, to implement dequeue bulking.
>>
>> RFC V3: Keeping the ball rolling.
>>
>> This patchset should now use BQL correctly. I've done lots of testing
>> for Head-of-Line blocking issues that can occur due to requeue of a
>> SKB bulk list. I've not been able to provoke any HoL blocking
>> situation, simply because BQL is doing such a good job, thus I'm
>> unable to "overshoot" HW/BQL limits with more than a single packet.
>>
>> This patch chooses a very conservative approach, as by default only
>> allowing dequeue of one extra packet, besides the normal dequeue.
>>
>> Open questions:
>>
>> - How do we expose tuning to userspace?
>> Patch adds /proc/sys/net/core/qdisc_bulk_dequeue_limit but I don't like it...
>> Per device tunable?
>>
>
> bql is using /sys, of course ;)
>
> # grep . /sys/class/net/eth1/queues/tx-0/byte_queue_limits/*
> /sys/class/net/eth1/queues/tx-0/byte_queue_limits/hold_time:1000
> /sys/class/net/eth1/queues/tx-0/byte_queue_limits/inflight:0
> /sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit:113314
> /sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_max:1879048192
> /sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_min:0
>
> Maybe you could simply reuse byte_queue_limits/limit, I am not sure we
> need a specific tunable.
>
>
>> - Can/should we limit dequeue bulking to devices supporting BQL?
>>
>
> Yes please. This will be an incentive to get BQL on drivers.
>
I think we can refine the expectation a bit. Bulk qdisc dequeue should
work for devices that don't support BQL, but the packet dequeue limit
is fixed number of packets (we don't want to create new mechanisms to
track fragments or packets that are outstanding in device). However,
in order to get bulk device transmit for a device, I think it
reasonable to request that BQL is supported since whoever is adding
that is already mucking in driver TX path anyway.
Tom
>
>
^ permalink raw reply
* Re: [RFC net-next PATCH V3 2/2] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Tom Herbert @ 2014-09-20 0:22 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Linux Netdev List, David S. Miller, Eric Dumazet,
Hannes Frederic Sowa, Florian Westphal, Daniel Borkmann,
Jamal Hadi Salim, Alexander Duyck, John Fastabend, Dave Taht,
Toke Høiland-Jørgensen
In-Reply-To: <20140919204926.3231.1970.stgit@dragon>
On Fri, Sep 19, 2014 at 1:49 PM, Jesper Dangaard Brouer
<brouer@redhat.com> wrote:
> Based on DaveM's recent API work on dev_hard_start_xmit(), that allows
> sending/processing an entire skb list.
>
> This patch implements qdisc bulk dequeue, by allowing multiple packets
> to be dequeued in dequeue_skb().
>
> The optimization principle for this is two fold, (1) to amortize
> locking cost and (2) avoid expensive tailptr update for notifying HW.
> (1) Several packets are dequeued while holding the qdisc root_lock,
> amortizing locking cost over several packet. The dequeued SKB list is
> processed under the TXQ lock in dev_hard_start_xmit(), thus also
> amortizing the cost of the TXQ lock.
> (2) Further more, dev_hard_start_xmit() will utilize the skb->xmit_more
> API to delay HW tailptr update, which also reduces the cost per
> packet.
>
> One restriction of the new API is that every SKB must belong to the
> same TXQ. This patch takes the easy way out, by restricting bulk
> dequeue to qdisc's with the TCQ_F_ONETXQUEUE flag, that specifies the
> qdisc only have attached a single TXQ.
>
> Some detail about the flow; dev_hard_start_xmit() will process the skb
> list, and transmit packets individually towards the driver (see
> xmit_one()). In case the driver stops midway in the list, the
> remaining skb list is returned by dev_hard_start_xmit(). In
> sch_direct_xmit() this returned list is requeued by dev_requeue_skb().
>
> To avoid overshooting the HW limits, which results in requeuing, the
> patch limits the amount of bytes dequeued, based on the drivers BQL
> limits.
>
> It also see GSO and segmented GSO packets, as a seperate kind of SKB
> bulking, and thus tries to avoid and stop dequeuing when seeing a GSO
> packet (both real GSO and segmented GSO skb lists).
>
> Choosing a very conservative initial default value of 1 extra packet
> dequeue, if bulking is allowed.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>
> ---
> V2:
> - Restruct functions, split out functionality
> - Use BQL bytelimit to avoid overshooting driver limits
>
> V3:
> - Correct use of BQL
> - Some minor adjustments based on feedback.
> - Default setting only bulk dequeue 1 extra packet (2 packets).
>
> include/net/sch_generic.h | 2 +
> net/core/sysctl_net_core.c | 9 ++++++
> net/sched/sch_generic.c | 70 ++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 78 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 1e89b9a..da9324f 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -14,6 +14,8 @@ struct qdisc_walker;
> struct tcf_walker;
> struct module;
>
> +extern int qdisc_bulk_dequeue_limit;
> +
> struct qdisc_rate_table {
> struct tc_ratespec rate;
> u32 data[256];
> diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
> index cf9cd13..5505841 100644
> --- a/net/core/sysctl_net_core.c
> +++ b/net/core/sysctl_net_core.c
> @@ -361,6 +361,15 @@ static struct ctl_table net_core_table[] = {
> .mode = 0644,
> .proc_handler = proc_dointvec
> },
> + {
> + .procname = "qdisc_bulk_dequeue_limit",
> + .data = &qdisc_bulk_dequeue_limit,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .extra1 = &zero,
> + .extra2 = &ushort_max,
> + .proc_handler = proc_dointvec_minmax
> + },
> { }
> };
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 346ef85..eee2280 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -34,6 +34,9 @@
> const struct Qdisc_ops *default_qdisc_ops = &pfifo_fast_ops;
> EXPORT_SYMBOL(default_qdisc_ops);
>
> +// FIXME: Still undecided where to put this parameter...
> +int qdisc_bulk_dequeue_limit __read_mostly = 1;
> +
> /* Main transmission queue. */
>
> /* Modifications to data participating in scheduling must be protected with
> @@ -56,6 +59,67 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
> return 0;
> }
>
> +static inline bool qdisc_may_bulk(const struct Qdisc *qdisc)
> +{
> + return qdisc->flags & TCQ_F_ONETXQUEUE;
> +}
> +
> +static inline struct sk_buff *qdisc_dequeue_validate(struct Qdisc *qdisc)
> +{
> + struct sk_buff *skb = qdisc->dequeue(qdisc);
> +
> + if (skb != NULL)
> + skb = validate_xmit_skb(skb, qdisc_dev(qdisc));
> +
> + return skb;
> +}
> +
> +static inline struct sk_buff *qdisc_bulk_dequeue_skb(struct Qdisc *q,
> + struct sk_buff *head)
> +{
> + struct sk_buff *new, *skb = head;
> + struct netdev_queue *txq = q->dev_queue;
> + int bytelimit = netdev_tx_avail_queue(txq);
> + int limit = qdisc_bulk_dequeue_limit;
> +
> + bytelimit -= skb->len; /* incorrect len if skb->next, but exits below */
> +
> + // if (bytelimit < psched_mtu(qdisc_dev(q)) //proposed by fw
> + if (bytelimit < 0)
> + return head;
> +
> + do {
> + if (skb->next || skb_is_gso(skb)) {
> + /* Stop processing if the skb is already a skb
> + * list (e.g a segmented GSO packet, from
> + * below or func caller) or a real GSO packet
> + */
> + break;
> + }
> + new = q->dequeue(q);
> + if (new) {
> + bytelimit -= new->len; /* covers GSO len */
> + new = validate_xmit_skb(new, qdisc_dev(q));
> + if (!new)
> + break;
> + /* "new" can be a skb list after validate call
> + * above (GSO segmented), but it is okay to
> + * append it to current skb->next, because
> + * next round will exit in-case "new" were a
> + * skb list.
> + */
> + skb->next = new;
> + skb = new;
> + }
> + } while (new && --limit && (bytelimit >= 0));
> + skb = head;
> +
> + return skb;
> +}
> +
> +/* Note that dequeue_skb can possibly return a SKB list (via skb->next).
> + * A requeued skb (via q->gso_skb) can also be a SKB list.
> + */
> static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> {
> struct sk_buff *skb = q->gso_skb;
> @@ -71,9 +135,9 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
> skb = NULL;
> } else {
> if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
> - skb = q->dequeue(q);
> - if (skb)
> - skb = validate_xmit_skb(skb, qdisc_dev(q));
> + skb = qdisc_dequeue_validate(q);
> + if (skb && qdisc_may_bulk(q) && qdisc_bulk_dequeue_limit)
> + skb = qdisc_bulk_dequeue_skb(q, skb);
> }
This is hard to read. I don't understand why qdisc_bulk_dequeue_skb
needs to be a separate function. Single dequeue and bulk dequeue can
happen in one common loop. Maybe something like:
struct sk_buff *ret = NULL, **next = &ret;
bytelimit = qdisc_may_bulk(q) ? netdev_tx_avail_queue(txq) : 0;
do {
skb= q->dequeue;
if (!skb)
break;
*next = skb;
next = &skb->next;
bytelimit -= skb->len;
limit--;
} while (bytelimit > 0 && limit > 0);
> }
>
>
^ permalink raw reply
* Re: [PATCH net-next v4 2/2] bonding: Simplify the xmit function for modes that use xmit_hash
From: Mahesh Bandewar @ 2014-09-20 0:09 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Jay Vosburgh, Veaceslav Falico, Andy Gospodarek, David Miller,
netdev, Eric Dumazet, Maciej Zenczykowski
In-Reply-To: <541C0E2A.3050309@redhat.com>
On Fri, Sep 19, 2014 at 4:06 AM, Nikolay Aleksandrov <nikolay@redhat.com> wrote:
>
> On 09/19/2014 12:00 PM, Nikolay Aleksandrov wrote:
> > On 09/18/2014 11:53 PM, Mahesh Bandewar wrote:
> >> Earlier change to use usable slave array for TLB mode had an additional
> >> performance advantage. So extending the same logic to all other modes
> >> that use xmit-hash for slave selection (viz 802.3AD, and XOR modes).
> >> Also consolidating this with the earlier TLB change.
> >>
> >> The main idea is to build the usable slaves array in the control path
> >> and use that array for slave selection during xmit operation.
> >>
> >> Measured performance in a setup with a bond of 4x1G NICs with 200
> >> instances of netperf for the modes involved (3ad, xor, tlb)
> >> cmd: netperf -t TCP_RR -H <TargetHost> -l 60 -s 5
> >>
> >> Mode TPS-Before TPS-After
> >>
> >> 802.3ad : 468,694 493,101
> >> TLB (lb=0): 392,583 392,965
> >> XOR : 475,696 484,517
> >>
> >> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
> >> ---
> >> v1:
> >> (a) If bond_update_slave_arr() fails to allocate memory, it will overwrite
> >> the slave that need to be removed.
> >> (b) Freeing of array will assign NULL (to handle bond->down to bond->up
> >> transition gracefully.
> >> (c) Change from pr_debug() to pr_err() if bond_update_slave_arr() returns
> >> failure.
> >> (d) XOR: bond_update_slave_arr() will consider mii-mon, arp-mon cases and
> >> will populate the array even if these parameters are not used.
> >> (e) 3AD: Should handle the ad_agg_selection_logic correctly.
> >> v2:
> >> (a) Removed rcu_read_{un}lock() calls from array manipulation code.
> >> (b) Slave link-events now refresh array for all these modes.
> >> (c) Moved free-array call from bond_close() to bond_uninit().
> >> v3:
> >> (a) Fixed null pointer dereference.
> >> (b) Removed bond->lock lockdep dependency.
> >> v4:
> >> (a) Made to changes to comply with Nikolay's locking changes
> >> (b) Added a work-queue to refresh slave-array when RTNL is not held
> >> (c) Array refresh happens ONLY with RTNL now.
> >> (d) alloc changed from GFP_ATOMIC to GFP_KERNEL
> >>
> <<<snip>>>
> >> @@ -3839,6 +4003,7 @@ static void bond_uninit(struct net_device *bond_dev)
> >> struct bonding *bond = netdev_priv(bond_dev);
> >> struct list_head *iter;
> >> struct slave *slave;
> >> + struct bond_up_slave *arr;
> >>
> >> bond_netpoll_cleanup(bond_dev);
> >>
> >> @@ -3847,6 +4012,12 @@ static void bond_uninit(struct net_device *bond_dev)
> >> __bond_release_one(bond_dev, slave->dev, true);
> >> netdev_info(bond_dev, "Released all slaves\n");
> >>
> Sorry but I just spotted a major problem, bond_3ad_unbind_slave() (called
> from __bond_release_one) calls ad_agg_selection_logic() which can re-arm
> the slave_arr work after it's supposed to be stopped here (i.e. the bond
> device has been closed so all works should've been stopped) so we might
> leak memory and access freed memory after all since it'll keep
> re-scheduling itself until it can acquire rtnl which is after the bond
> device has been destroyed.
>
This should not be a problem. ndo_close (bond_close()) is called
before ndo_uninit(bond_uninit()), so the work-queues get cancelled
there so if rearm tries to schedule some work after queue gets
cancelled, it can't do much and wont harm anything.
Hence there wont be any arrays built once it's free-ed completely and
therefore no memory leak. I addded some instrumentation and tried
following sequence -
# modprobe bonding mode=4
# ip link set bond0 up
# [Add ip]
# [Add default route]
# ifenslave bond0 eth0 eth1 eth2 eth3
....
[Run some backgound traffic. I used netperf.]
# ip link bond0 down
I did not see anything "bad" happening. Did your trial produced
something unpleasant?
> >> + arr = rtnl_dereference(bond->slave_arr);
> >> + if (arr) {
> >> + kfree_rcu(arr, rcu);
> >> + RCU_INIT_POINTER(bond->slave_arr, NULL);
> >> + }
> >> +
> >> list_del(&bond->bond_list);
> >>
> >> bond_debug_unregister(bond);
> >> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
> >> index 98dc0d7ad731..4635b175256a 100644
> >> --- a/drivers/net/bonding/bonding.h
> >> +++ b/drivers/net/bonding/bonding.h
> >> @@ -177,6 +177,12 @@ struct slave {
> >> struct kobject kobj;
> >> };
> >>
> >> +struct bond_up_slave {
> >> + unsigned int count;
> >> + struct rcu_head rcu;
> >> + struct slave *arr[0];
> >> +};
> >> +
> >> /*
> >> * Link pseudo-state only used internally by monitors
> >> */
> >> @@ -191,6 +197,7 @@ struct bonding {
> >> struct slave __rcu *curr_active_slave;
> >> struct slave __rcu *current_arp_slave;
> >> struct slave __rcu *primary_slave;
> >> + struct bond_up_slave __rcu *slave_arr; /* Array of usable slaves */
> >> bool force_primary;
> >> s32 slave_cnt; /* never change this value outside the attach/detach wrappers */
> >> int (*recv_probe)(const struct sk_buff *, struct bonding *,
> >> @@ -220,6 +227,7 @@ struct bonding {
> >> struct delayed_work alb_work;
> >> struct delayed_work ad_work;
> >> struct delayed_work mcast_work;
> >> + struct delayed_work slave_arr_work;
> >> #ifdef CONFIG_DEBUG_FS
> >> /* debugging support via debugfs */
> >> struct dentry *debug_dir;
> >> @@ -531,6 +539,8 @@ const char *bond_slave_link_status(s8 link);
> >> struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> >> struct net_device *end_dev,
> >> int level);
> >> +int bond_update_slave_arr(struct bonding *bond, struct slave *skipslave);
> >> +void bond_slave_arr_work_rearm(struct bonding *bond);
> >>
> >> #ifdef CONFIG_PROC_FS
> >> void bond_create_proc_entry(struct bonding *bond);
> >>
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe netdev" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
^ permalink raw reply
* Re: [net-next PATCH 00/29] Add support for the Intel FM10000 Ethernet Switch Host Interface
From: Alexander Duyck @ 2014-09-19 23:52 UTC (permalink / raw)
To: davem
Cc: Alexander Duyck, nhorman, netdev, john.fastabend, matthew.vick,
jeffrey.t.kirsher, sassmann
In-Reply-To: <20140918223242.10373.27403.stgit@ahduyck-bv4.jf.intel.com>
Dave,
I was wondering what changes you wanted for these patches? The only
feedback that I have gotten so far was on the last 2 patches, and a one
liner about dev_consume_skb_any vs dev_kfree_skb_any. Is there
something I missed, or do I need to resubmit the entire set with some
other sort of fix?
I was hoping you could just pull the first 27, and then I could resubmit
the last 2 with an attempt at addressing formatting/comment fixes based
on Richard's comments as well as a small patch for the
dev_consume_skb_any change needed in the transmit cleanup.
Thanks,
Alex
On 09/18/2014 03:35 PM, Alexander Duyck wrote:
> This patch series adds support for the FM10000 Ethernet switch host
> interface. The Intel FM10000 Ethernet Switch is a 48-port Ethernet switch
> supporting both Ethernet ports and PCI Express host interfaces. The fm10k
> driver provides support for the host interface portion of the switch, both
> PF and VF.
>
> As the host interfaces are directly connected to the switch this results in
> some significant differences versus a standard network driver. For example
> there is no PHY or MII on the device. Since packets are delivered directly
> from the switch to the host interface these are unnecessary. Otherwise most
> of the functionality is very similar to our other network drivers such as
> ixgbe or igb. For example we support all the standard network offloads,
> jumbo frames, SR-IOV (64 VFS), PTP, and some VXLAN and NVGRE offloads.
>
> ---
>
> Alexander Duyck (29):
> fm10k: Add skeletal frame for Intel(R) FM10000 Ethernet Switch Host Interface Driver
> fm10k: Add register defines and basic structures
> fm10k: Add support for TLV message parsing and generation
> fm10k: Add support for basic interaction with hardware
> fm10k: Add support for mailbox
> fm10k: Implement PF <-> SM mailbox operations
> fm10k: Add support for PF
> fm10k: Add support for configuring PF interface
> fm10k: Add netdev
> fm10k: Add support for L2 filtering
> fm10k: Add support for ndo_open/stop
> fm10k: Add interrupt support
> fm10k: add support for Tx/Rx rings
> fm10k: Add service task to handle delayed events
> fm10k: Add Tx/Rx hardware ring bring-up/tear-down
> fm10k: Add transmit and receive fastpath and interrupt handlers
> fm10k: Add ethtool support
> fm10k: Add support for PCI power management and error handling
> fm10k: Add support for multiple queues
> fm10k: Add support for netdev offloads
> fm10k: Add support for MACVLAN acceleration
> fm10k: Add support for PF <-> VF mailbox
> fm10k: Add support for VF
> fm10k: Add support for SR-IOV to PF core files
> fm10k: Add support for SR-IOV to driver
> fm10k: Add support for IEEE DCBx
> fm10k: Add support for debugfs
> fm10k: Add support for ptp to hw specific files
> fm10k: Add support for PTP
>
>
> drivers/net/ethernet/intel/Kconfig | 19
> drivers/net/ethernet/intel/Makefile | 1
> drivers/net/ethernet/intel/fm10k/Makefile | 33
> drivers/net/ethernet/intel/fm10k/fm10k.h | 532 +++++
> drivers/net/ethernet/intel/fm10k/fm10k_common.c | 534 +++++
> drivers/net/ethernet/intel/fm10k/fm10k_common.h | 65 +
> drivers/net/ethernet/intel/fm10k/fm10k_dcbnl.c | 174 ++
> drivers/net/ethernet/intel/fm10k/fm10k_debugfs.c | 259 +++
> drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c | 1069 +++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_iov.c | 536 +++++
> drivers/net/ethernet/intel/fm10k/fm10k_main.c | 1978 ++++++++++++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_mbx.c | 2125 ++++++++++++++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_mbx.h | 307 +++
> drivers/net/ethernet/intel/fm10k/fm10k_netdev.c | 1424 ++++++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_pci.c | 2166 ++++++++++++++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 1849 +++++++++++++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_pf.h | 135 +
> drivers/net/ethernet/intel/fm10k/fm10k_ptp.c | 535 +++++
> drivers/net/ethernet/intel/fm10k/fm10k_tlv.c | 863 +++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_tlv.h | 186 ++
> drivers/net/ethernet/intel/fm10k/fm10k_type.h | 769 ++++++++
> drivers/net/ethernet/intel/fm10k/fm10k_vf.c | 552 ++++++
> drivers/net/ethernet/intel/fm10k/fm10k_vf.h | 78 +
> 23 files changed, 16189 insertions(+)
> create mode 100644 drivers/net/ethernet/intel/fm10k/Makefile
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k.h
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_common.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_common.h
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_dcbnl.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_debugfs.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_ethtool.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_iov.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_main.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_mbx.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_mbx.h
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_netdev.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_pci.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_pf.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_pf.h
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_ptp.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_tlv.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_tlv.h
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_type.h
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_vf.c
> create mode 100644 drivers/net/ethernet/intel/fm10k/fm10k_vf.h
>
^ permalink raw reply
* [PATCH] scsi: fix kconfig dependency warnings for SCSI_FC_ATTRS
From: Randy Dunlap @ 2014-09-19 23:50 UTC (permalink / raw)
To: netdev@vger.kernel.org, David Miller, linux-scsi
Cc: James Bottomley, Anish Bhatt
From: Randy Dunlap <rdunlap@infradead.org>
Fix kconfig dependency warnings which can lead to build errors:
warning: (SCSI_BNX2X_FCOE && LIBFCOE && TCM_QLA2XXX) selects LIBFC which has unmet direct dependencies (SCSI_LOWLEVEL && SCSI && SCSI_FC_ATTRS)
warning: (FCOE && FCOE_FNIC) selects LIBFCOE which has unmet direct dependencies (SCSI_LOWLEVEL && SCSI && SCSI_FC_ATTRS)
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
---
drivers/scsi/Kconfig | 3 +++
drivers/scsi/qla2xxx/Kconfig | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
--- linux-next-20140918.orig/drivers/scsi/Kconfig
+++ linux-next-20140918/drivers/scsi/Kconfig
@@ -601,6 +601,7 @@ config LIBFC
config LIBFCOE
tristate "LibFCoE module"
+ depends on SCSI_FC_ATTRS
select LIBFC
---help---
Library for Fibre Channel over Ethernet module
@@ -608,6 +609,7 @@ config LIBFCOE
config FCOE
tristate "FCoE module"
depends on PCI
+ depends on SCSI_FC_ATTRS
select LIBFCOE
---help---
Fibre Channel over Ethernet module
@@ -615,6 +617,7 @@ config FCOE
config FCOE_FNIC
tristate "Cisco FNIC Driver"
depends on PCI && X86
+ depends on SCSI_FC_ATTRS
select LIBFCOE
help
This is support for the Cisco PCI-Express FCoE HBA.
--- linux-next-20140918.orig/drivers/scsi/qla2xxx/Kconfig
+++ linux-next-20140918/drivers/scsi/qla2xxx/Kconfig
@@ -30,7 +30,7 @@ config SCSI_QLA_FC
config TCM_QLA2XXX
tristate "TCM_QLA2XXX fabric module for Qlogic 2xxx series target mode HBAs"
- depends on SCSI_QLA_FC && TARGET_CORE
+ depends on SCSI_QLA_FC && TARGET_CORE && SCSI_FC_ATTRS
select LIBFC
select BTREE
default n
^ permalink raw reply
* Re: [PATCH net] macvlan: allow to enqueue broadcast pkt on virtual device
From: Eric Dumazet @ 2014-09-19 23:44 UTC (permalink / raw)
To: Herbert Xu; +Cc: David Miller, nicolas.dichtel, netdev
In-Reply-To: <20140919233108.GA31226@gondor.apana.org.au>
On Sat, 2014-09-20 at 07:31 +0800, Herbert Xu wrote:
> While it would be nice to maintain the ability to chnge this
> parameter, I'm fine with this patch as it stands since it fixes
> a real regression.
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Another interesting feature would be to be able to time limit the queue,
and/or use a head drop instead of tail drop.
(ie timestamp skb when its enqueued into bc_queue,
and at dequeue time, drop it if elapsed time is above a configurable
limit)
Processing a broadcast one minute after it was really sent makes little
sense, especially if we drop new incoming broadcasts because bc_queue is
full.
^ permalink raw reply
* Re: [PATCH v2 2/2] tg3: Allow for receive of full-size 8021AD frames
From: Eric Dumazet @ 2014-09-19 23:37 UTC (permalink / raw)
To: Prashant Sreedharan
Cc: Vladislav Yasevich, netdev, Vladislav Yasevich, Michael Chan
In-Reply-To: <1411168475.6401.37.camel@prashant>
On Fri, 2014-09-19 at 16:14 -0700, Prashant Sreedharan wrote:
> The NIC will not receive frames of size > mtu + ETH_HLEN + VLAN_HLEN it
> will update the oversize_frame count and drop it. The check is to make
> sure the additional 4 bytes is due to a valid vlan header.
I see, thanks for the explanation ;)
^ 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