Netdev List
 help / color / mirror / Atom feed
* [RFC PATCH v2 net-next 0/2] BPF and OVS extensions
From: Alexei Starovoitov @ 2013-09-17  2:48 UTC (permalink / raw)
  To: David S. Miller, netdev-u79uwXL29TY76Z2rM5mHXA, Eric Dumazet,
	Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
	Patrick McHardy, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	Daniel Borkmann, Paul E. McKenney, Xi Wang, David Howells,
	Cong Wang, Jesse Gross, Pravin B Shelar, Ben Pfaff, Thomas Graf,
	dev-yBygre7rU0TnMu66kgdUjQ

while net-next is closed, collecting feedback...

V2:
No changes to BPF engine
No changes to uapi
Add static branch prediction markings, remove unnecessary safety checks,
fix crash where packets were enqueued to a BPF program while program
was being unloaded

V1:
Today OVS is a cache engine. Userspace controller simulates traversal of
network topology and establishes a flow (cached result of the traversal).
Suffering upcall penalty, flow explosion, flow invalidation on topology
changes, difficulties in keeping inner topology stats, etc. This patch
enhances OVS by moving simple cases of topology traversal next to the packet.
On a flow miss the chain of BPF programs executes the network topology.
If packet requires userspace processing it can be pushed up by BPF program.
BPF program that represent a bridge just needs to forward packets.
MAC learning can be done either by BPF program or via userpsace upcall.
Such bridge/router/nat can be programmed in BPF.
To achieve that BPF was extended to allow easier programability in restricted C
or in dataplane language.

Patch 1/2: generic BPF extension
Original A and X 32-bit BPF registers are replaced with ten 64-bit registers.
bpf opcode encoding kept the same. load/store were generalized to access stack,
bpf_tables and bpf_context.
BPF program interfaces to outside world via tables that it can read and write,
and via bpf_context which is in/out blob of data.
Other kernel components can provide callbacks to tailor BPF to specific needs.

Patch 2/2: extends OVS with network functions that use BPF as execution engine

BPF backend for GCC is available at:
https://github.com/iovisor/bpf_gcc

Distributed bridge demo written in BPF:
https://github.com/iovisor/iovisor

Alexei Starovoitov (2):
  extended BPF
  extend OVS to use BPF programs on flow miss

 arch/x86/net/Makefile            |    2 +-
 arch/x86/net/bpf2_jit_comp.c     |  610 +++++++++++++++++++
 arch/x86/net/bpf_jit_comp.c      |   41 +-
 arch/x86/net/bpf_jit_comp.h      |   36 ++
 include/linux/filter.h           |   79 +++
 include/uapi/linux/filter.h      |  125 +++-
 include/uapi/linux/openvswitch.h |  140 +++++
 net/core/Makefile                |    2 +-
 net/core/bpf_check.c             | 1043 ++++++++++++++++++++++++++++++++
 net/core/bpf_run.c               |  412 +++++++++++++
 net/openvswitch/Makefile         |    7 +-
 net/openvswitch/bpf_callbacks.c  |  295 +++++++++
 net/openvswitch/bpf_plum.c       |  931 +++++++++++++++++++++++++++++
 net/openvswitch/bpf_replicator.c |  155 +++++
 net/openvswitch/bpf_table.c      |  500 ++++++++++++++++
 net/openvswitch/datapath.c       |  102 +++-
 net/openvswitch/datapath.h       |    5 +
 net/openvswitch/dp_bpf.c         | 1228 ++++++++++++++++++++++++++++++++++++++
 net/openvswitch/dp_bpf.h         |  160 +++++
 net/openvswitch/dp_notify.c      |    7 +
 net/openvswitch/vport-gre.c      |   10 -
 net/openvswitch/vport-netdev.c   |   15 +-
 net/openvswitch/vport-netdev.h   |    1 +
 net/openvswitch/vport.h          |   10 +
 24 files changed, 5854 insertions(+), 62 deletions(-)
 create mode 100644 arch/x86/net/bpf2_jit_comp.c
 create mode 100644 arch/x86/net/bpf_jit_comp.h
 create mode 100644 net/core/bpf_check.c
 create mode 100644 net/core/bpf_run.c
 create mode 100644 net/openvswitch/bpf_callbacks.c
 create mode 100644 net/openvswitch/bpf_plum.c
 create mode 100644 net/openvswitch/bpf_replicator.c
 create mode 100644 net/openvswitch/bpf_table.c
 create mode 100644 net/openvswitch/dp_bpf.c
 create mode 100644 net/openvswitch/dp_bpf.h

-- 
1.7.9.5

^ permalink raw reply

* Re: [PATCH net-next] net loopback: Set loopback_dev to NULL when freed
From: Eric W. Biederman @ 2013-09-17  1:52 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, edumazet, jiri, alexander.h.duyck, amwang, netdev,
	fruggeri
In-Reply-To: <1379382102.4751.2.camel@edumazet-glaptop>

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Mon, 2013-09-16 at 21:34 -0400, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Mon, 16 Sep 2013 17:50:51 -0700
>> 
>> > On Mon, 2013-09-16 at 16:52 -0700, Eric W. Biederman wrote:
>> >> It has recently turned up that we have a number of long standing bugs
>> >> in the network stack cleanup code with use of the loopback device
>> >> after it has been freed that have not turned up because in most cases
>> >> the storage allocated to the loopback device is not reused, when those
>> >> accesses happen.
>> >> 
>> >> Set looback_dev to NULL to trigger oopses instead of silent data corrupt
>> >> when we hit this class of bug.
>> >> 
>> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> >> ---
>> > 
>> > Acked-by: Eric Dumazet <edumazet@google.com>
>> 
>> I'd like to apply this to 'net', any objections?
>
> No objections from me.

No objects from me I just hadn't seen it as a bug fix, but I guess it
sort of is.

Eric

^ permalink raw reply

* Re: [PATCH v3 net-next 21/27] net: add a function to get the next private
From: Ben Hutchings @ 2013-09-17  1:50 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: netdev, jiri, David S. Miller, Eric Dumazet, Alexander Duyck
In-Reply-To: <1379378812-18346-22-git-send-email-vfalico@redhat.com>

On Tue, 2013-09-17 at 02:46 +0200, Veaceslav Falico wrote:
> It searches for the provided private and returns the next one. If private
> is not found or next list element is list head - returns NULL.

This is going to take linear time, which is probably OK for a bond that
has only a very few devices.  But it would likely be a really bad idea
for, say, a bridge device that could have tens or hundreds of lower
devices.  So it's not a generically useful function.

I think the bonding driver can implement this:

[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5055,6 +5055,33 @@ void *netdev_lower_dev_get_private(struct net_device *dev,
>  }
>  EXPORT_SYMBOL(netdev_lower_dev_get_private);
>  
> +/* netdev_lower_dev_get_next_private - return the ->private of the list
> + *				       element whos ->private == private.
> + * @dev - device to search
> + * @private - private pointer to search for.
> + *
> + * Returns the next ->private pointer, if ->next is not head and private is
> + * found.
> + */
> +extern void *netdev_lower_dev_get_next_private(struct net_device *dev,
> +					       void *private)
> +{
> +	struct netdev_adjacent *lower;
> +
> +	list_for_each_entry(lower, &dev->adj_list.lower, list) {
> +		if (lower->private == private) {
> +			lower = list_entry(lower->list.next,
> +					   struct netdev_adjacent, list);
> +			if (&lower->list == &dev->adj_list.lower)
> +				return NULL;
> +			return lower->private;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(netdev_lower_dev_get_next_private);

using only the functions already exported:

static void *__bond_next_slave(struct net_device *dev, void *private)
{
	struct list_head *iter;
	struct net_device *lower;
	bool found = false;

	netdev_for_each_lower_dev(dev, lower, iter) {
		if (found)
			return netdev_adjacent_get_private(iter);
		if (netdev_adjacent_get_private(iter) == private)
			found = true;
	}

	return NULL;
}

(not that I've tested it :-).

Ben.

> +
>  static void dev_change_rx_flags(struct net_device *dev, int flags)
>  {
>  	const struct net_device_ops *ops = dev->netdev_ops;

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply

* [PATCH net] tcp: fix RTO calculated from cached RTT
From: Neal Cardwell @ 2013-09-17  1:44 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Neal Cardwell, Eric Dumazet, Yuchung Cheng

Commit 1b7fdd2ab5852 ("tcp: do not use cached RTT for RTT estimation")
did not correctly account for the fact that crtt is the RTT shifted
left 3 bits. Fix the calculation to consistently reflect this fact.

Signed-off-by: Neal Cardwell <ncardwell@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
---
 net/ipv4/tcp_metrics.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 4a22f3e..52f3c6b 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -502,7 +502,9 @@ reset:
 	 * ACKs, wait for troubles.
 	 */
 	if (crtt > tp->srtt) {
-		inet_csk(sk)->icsk_rto = crtt + max(crtt >> 2, tcp_rto_min(sk));
+		/* Set RTO like tcp_rtt_estimator(), but from cached RTT. */
+		crtt >>= 3;
+		inet_csk(sk)->icsk_rto = crtt + max(2 * crtt, tcp_rto_min(sk));
 	} else if (tp->srtt == 0) {
 		/* RFC6298: 5.7 We've failed to get a valid RTT sample from
 		 * 3WHS. This is most likely due to retransmission,
-- 
1.8.4

^ permalink raw reply related

* Re: [PATCH RFC net] msi: free msi_desc entry only after we've released the kobject
From: Veaceslav Falico @ 2013-09-17  1:46 UTC (permalink / raw)
  To: Veaceslav Falico; +Cc: netdev, Neil Horman, Russell King, Bjorn Helgaas
In-Reply-To: <1379351396-6458-1-git-send-email-vfalico@redhat.com>

On Mon, Sep 16, 2013 at 7:09 PM, Veaceslav Falico <vfalico@redhat.com> wrote:
> Currently, we first do kobject_put(&entry->kobj) and the kfree(entry),
> however kobject_put() doesn't guarantee us that it was the last reference
> and that the kobj isn't used currently by someone else, so after we
> kfree(entry) with the struct kobject - other users will begin using the
> freed memory, instead of the actual kobject.
>
> Fix this by using the kobject->release callback, which is called last when
> the kobject is indeed not used and is cleaned up - it's msi_kobj_release(),
> which can do the kfree(entry) safely (kobject_put/cleanup doesn't use the
> kobj itself after ->release() was called, so we're safe).
>
> Also, in case we've failed to create the sysfs directories - just kfree()
> it - cause we don't have the kobjects attached.
>
> CC: Neil Horman <nhorman@tuxdriver.com>
> CC: Russell King <rmk+kernel@arm.linux.org.uk>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>
> Notes:
>     This patch is really an RFC, and I don't know for sure how to correctly
>     fix it, however it seems to work. Sorry if I've done something horribly
>     wrong, it really seems to work ok :).

Sorry, done two things horribly wrong - wrong list and
still a bit buggy patch.

Will send a new version to the appropriate lists :).

>
>     I've hit the bug with the recent CONFIG_DEBUG_KOBJECT_RELEASE - it basically
>     delays the cleanup a bit - so that the chances are a lot higher even for
>     one user to hit it.
>
>     Or, maybe, it will be better to just add an kobject helper
>     kobject_wait_cleanup(), which will return only after it's indeed free? I'm
>     really not sure.
>
>  drivers/pci/msi.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index b35f93c..6eabf93 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -395,6 +395,7 @@ static void free_msi_irqs(struct pci_dev *dev)
>                         if (list_is_last(&entry->list, &dev->msi_list))
>                                 iounmap(entry->mask_base);
>                 }
> +               list_del(&entry->list);
>
>                 /*
>                  * Its possible that we get into this path
> @@ -405,10 +406,9 @@ static void free_msi_irqs(struct pci_dev *dev)
>                 if (entry->kobj.parent) {
>                         kobject_del(&entry->kobj);
>                         kobject_put(&entry->kobj);
> +               } else {
> +                       kfree(entry);
>                 }
> -
> -               list_del(&entry->list);
> -               kfree(entry);
>         }
>  }
>
> @@ -531,6 +531,7 @@ static void msi_kobj_release(struct kobject *kobj)
>         struct msi_desc *entry = to_msi_desc(kobj);
>
>         pci_dev_put(entry->dev);
> +       kfree(entry);
>  }
>
>  static struct kobj_type msi_irq_ktype = {
> --
> 1.8.4
>
> --
> 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: Pull request: sfc 2013-09-17
From: David Miller @ 2013-09-17  1:44 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1379376611.1945.11.camel@bwh-desktop.uk.level5networks.com>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 17 Sep 2013 01:10:11 +0100

>   git://git.kernel.org/pub/scm/linux/kernel/git/bwh/sfc.git sfc-3.12
 ...
> Some bug fixes and future-proofing for the recently added SFC9120
> support:
> 
> 1. Minimal support for the 40G configuration.
> 2. Disable the incomplete PTP/hardware timestamping support.
> 3. Reset MAC stats properly after a firmware upgrade.
> 4. Re-check the datapath firmware capabilities after the controller is
> reset.

Pulled, thanks Ben.

^ permalink raw reply

* Re: [PATCH net-next] net loopback: Set loopback_dev to NULL when freed
From: Eric Dumazet @ 2013-09-17  1:41 UTC (permalink / raw)
  To: David Miller
  Cc: ebiederm, edumazet, jiri, alexander.h.duyck, amwang, netdev,
	fruggeri
In-Reply-To: <20130916.213435.1508866100258405440.davem@davemloft.net>

On Mon, 2013-09-16 at 21:34 -0400, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 16 Sep 2013 17:50:51 -0700
> 
> > On Mon, 2013-09-16 at 16:52 -0700, Eric W. Biederman wrote:
> >> It has recently turned up that we have a number of long standing bugs
> >> in the network stack cleanup code with use of the loopback device
> >> after it has been freed that have not turned up because in most cases
> >> the storage allocated to the loopback device is not reused, when those
> >> accesses happen.
> >> 
> >> Set looback_dev to NULL to trigger oopses instead of silent data corrupt
> >> when we hit this class of bug.
> >> 
> >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> ---
> > 
> > Acked-by: Eric Dumazet <edumazet@google.com>
> 
> I'd like to apply this to 'net', any objections?

No objections from me.

^ permalink raw reply

* Re: [PATCH net] net: sctp: rfc4443: do not report ICMP redirects to user space
From: David Miller @ 2013-09-17  1:40 UTC (permalink / raw)
  To: dborkman; +Cc: netdev, linux-sctp, hannes
In-Reply-To: <1379327762-4638-1-git-send-email-dborkman@redhat.com>

From: Daniel Borkmann <dborkman@redhat.com>
Date: Mon, 16 Sep 2013 12:36:02 +0200

> Adapt the same behaviour for SCTP as present in TCP for ICMP redirect
> messages. For IPv6, RFC4443, section 2.4. says:
> 
>   ...
>   (e) An ICMPv6 error message MUST NOT be originated as a result of
>       receiving the following:
>   ...
>        (e.2) An ICMPv6 redirect message [IPv6-DISC].
>   ...
> 
> Therefore, do not report an error to user space, just invoke dst's redirect
> callback and leave, same for IPv4 as done in TCP as well. The implication
> w/o having this patch could be that the reception of such packets would
> generate a poll notification and in worst case it could even tear down the
> whole connection. Therefore, stop updating sk_err on redirects.
> 
> Reported-by: Duan Jiong <duanj.fnst@cn.fujitsu.com>
> Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Suggested-by: Vlad Yasevich <vyasevich@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>

Applied and queued up for -stable, thanks Daniel.

^ permalink raw reply

* Re: [PATCH 0/3] net: usb: cdc_ether: improve telit support and code cleanups
From: David Miller @ 2013-09-17  1:38 UTC (permalink / raw)
  To: fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w
  Cc: oliver-GvhC2dPhHPQdnm+yROfE0A, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1379324872-15944-1-git-send-email-fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

From: Fabio Porcedda <fabio.porcedda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Mon, 16 Sep 2013 11:47:49 +0200

> Some patches to improve telit modules support and to cleanup the code.

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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] ip6_tunnels: raddr and laddr are inverted in nl msg
From: David Miller @ 2013-09-17  1:36 UTC (permalink / raw)
  To: nicolas.dichtel; +Cc: netdev, zhi.ding
In-Reply-To: <1379323875-3794-1-git-send-email-nicolas.dichtel@6wind.com>

From: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Date: Mon, 16 Sep 2013 11:31:15 +0200

> From: Ding Zhi <zhi.ding@6wind.com>
> 
> IFLA_IPTUN_LOCAL and IFLA_IPTUN_REMOTE were inverted.
> 
> Introduced by c075b13098b3 (ip6tnl: advertise tunnel param via rtnl).
> 
> Signed-off-by: Ding Zhi <zhi.ding@6wind.com>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>

Applied and queued up for -stable, thanks.

^ permalink raw reply

* Re: [PATCH net-next] net loopback: Set loopback_dev to NULL when freed
From: David Miller @ 2013-09-17  1:34 UTC (permalink / raw)
  To: eric.dumazet
  Cc: ebiederm, edumazet, jiri, alexander.h.duyck, amwang, netdev,
	fruggeri
In-Reply-To: <1379379051.4751.0.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 16 Sep 2013 17:50:51 -0700

> On Mon, 2013-09-16 at 16:52 -0700, Eric W. Biederman wrote:
>> It has recently turned up that we have a number of long standing bugs
>> in the network stack cleanup code with use of the loopback device
>> after it has been freed that have not turned up because in most cases
>> the storage allocated to the loopback device is not reused, when those
>> accesses happen.
>> 
>> Set looback_dev to NULL to trigger oopses instead of silent data corrupt
>> when we hit this class of bug.
>> 
>> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> ---
> 
> Acked-by: Eric Dumazet <edumazet@google.com>

I'd like to apply this to 'net', any objections?

^ permalink raw reply

* Re: [PATCH] bgmac: implement unaligned addressing for DMA rings that support it
From: David Miller @ 2013-09-17  1:31 UTC (permalink / raw)
  To: zajec5; +Cc: netdev, hauke
In-Reply-To: <1379279598-25274-1-git-send-email-zajec5@gmail.com>

From: Rafał Miłecki <zajec5@gmail.com>
Date: Sun, 15 Sep 2013 23:13:18 +0200

> This is important patch for new devices that support unaligned
> addressing. That devices suffer from the backward-compatibility bug in
> DMA engine. In theory we should be able to use old mechanism, but in
> practice DMA address seems to be randomly copied into status register
> when hardware reaches end of a ring. This breaks reading slot number
> from status register and we can't use DMA anymore.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>

Applied.

^ permalink raw reply

* Re: [PATCH V2] bgmac: allow bigger et_swtype nvram variable
From: David Miller @ 2013-09-17  1:31 UTC (permalink / raw)
  To: zajec5; +Cc: netdev, hauke
In-Reply-To: <1379278148-6966-1-git-send-email-zajec5@gmail.com>

From: Rafał Miłecki <zajec5@gmail.com>
Date: Sun, 15 Sep 2013 22:49:08 +0200

> From: Hauke Mehrtens <hauke@hauke-m.de>
> 
> Without this patch it is impossible to read et_swtype, because the 1
> byte space is needed for the terminating null byte. The max expected
> value is 0xF, so now it should be possible to read decimal form ("15")
> and hex form ("0xF").
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>

Applied.

^ permalink raw reply

* Re: [PATCH] bgmac: fix internal switch initialization
From: David Miller @ 2013-09-17  1:31 UTC (permalink / raw)
  To: zajec5; +Cc: netdev, hauke, stable
In-Reply-To: <1379197367-19116-1-git-send-email-zajec5@gmail.com>

From: Rafał Miłecki <zajec5@gmail.com>
Date: Sun, 15 Sep 2013 00:22:47 +0200

> Some devices (BCM4749, BCM5357, BCM53572) have internal switch that
> requires initialization. We already have code for this, but because
> of the typo in code it was never working. This resulted in network not
> working for some routers and possibility of soft-bricking them.
> 
> Use correct bit for switch initialization and fix typo in the define.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>

Applied.

^ permalink raw reply

* Re: [patch 1/4] drivers/net/ethernet/ibm/ehea/ehea_main.c: add alias entry for portN properties
From: David Miller @ 2013-09-17  1:29 UTC (permalink / raw)
  To: cascardo; +Cc: akpm, netdev, ohering, jeffm, jslaby
In-Reply-To: <20130916210141.GB14221@oc0268524204.ibm.com>

From: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>
Date: Mon, 16 Sep 2013 18:01:41 -0300

> Sorry for the noise. I just checked again the bug, and had to read
> between the lines that this issue might happen with the generation of
> initrd, when the scripts check for /sys/class/net/eth0/device/modalias,
> which links to the port device at
> /sys/devices/ibmebus/23c00400.lhea/port0/.
> 
> I think that should be clarified in the log message. Besides that, since
> this has also been in the field for a long time:
> 
> Acked-by: Thadeu Lima de Souza Cascardo <cascardo@linux.vnet.ibm.com>

I adjusted the commit message and applied this, thanks.

^ permalink raw reply

* Re: [PATCH v3 net-next 07/27] net: add for_each iterators through neighbour lower link's private
From: Ben Hutchings @ 2013-09-17  1:26 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: netdev, jiri, David S. Miller, Eric Dumazet, Alexander Duyck
In-Reply-To: <1379378812-18346-8-git-send-email-vfalico@redhat.com>

On Tue, 2013-09-17 at 02:46 +0200, Veaceslav Falico wrote:
[...]
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4537,6 +4537,72 @@ struct net_device *netdev_all_upper_get_next_dev_rcu(struct net_device *dev,
>  }
>  EXPORT_SYMBOL(netdev_all_upper_get_next_dev_rcu);
>  
> +/* netdev_lower_get_next_private - Get the next ->private from the
> + *				   lower neighbour list
[...]

This is not correct kernel-doc syntax.  You must begin the comment like
this:

/**
 * function_name - summary on one physical line, no wrapping allowed

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply

* Re: [PATCH] net, mellanox mlx4 Fix compile warnings
From: David Miller @ 2013-09-17  1:25 UTC (permalink / raw)
  To: prarit; +Cc: netdev, dledford, amirv, ogerlitz
In-Reply-To: <1379075438-20240-1-git-send-email-prarit@redhat.com>

From: Prarit Bhargava <prarit@redhat.com>
Date: Fri, 13 Sep 2013 08:30:38 -0400

> @@ -2563,7 +2563,7 @@ int mlx4_HW2SW_CQ_wrapper(struct mlx4_dev *dev, int slave,
>  {
>  	int err;
>  	int cqn = vhcr->in_modifier;
> -	struct res_cq *cq;
> +	struct res_cq *uninitialized_var(cq);
>  
>  	err = cq_res_start_move_to(dev, slave, cqn, RES_CQ_ALLOCATED, &cq);
>  	if (err)

This gross annotation is not necessary.

Make cq_res_start_move_to() return 'cq' as an error pointer instead of
setting 'cq' by reference.

^ permalink raw reply

* Re: [PATCH] net, vxlan Fix compile warning
From: David Miller @ 2013-09-17  1:23 UTC (permalink / raw)
  To: prarit; +Cc: netdev, jpirko, tgraf
In-Reply-To: <1379075352-20051-1-git-send-email-prarit@redhat.com>

From: Prarit Bhargava <prarit@redhat.com>
Date: Fri, 13 Sep 2013 08:29:12 -0400

> Fix a unintialized variable warning.
> 
> drivers/net/vxlan.c: In function ‘vxlan_sock_add’:
> drivers/net/vxlan.c:2240:11: error: ‘sock’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   vs->sock = sock;
>            ^
> drivers/net/vxlan.c:2217:17: note: ‘sock’ was declared here
>   struct socket *sock;
>                  ^
> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>

Make create_v{4,6}_sock() return an err pointer.

Problem solved and no incredibly ugly, unnecessary, annotations.

The error code return from these functions already gets turned into
and error pointer at the call site, which makes this situation even
more rediculous.


^ permalink raw reply

* Re: [PATCH v3 net-next 02/27] net: add RCU variant to search for netdev_adjacent link
From: Ben Hutchings @ 2013-09-17  1:23 UTC (permalink / raw)
  To: Veaceslav Falico
  Cc: netdev, jiri, David S. Miller, Eric Dumazet, Alexander Duyck,
	Cong Wang
In-Reply-To: <1379378812-18346-3-git-send-email-vfalico@redhat.com>

On Tue, 2013-09-17 at 02:46 +0200, Veaceslav Falico wrote:
> Currently we have only the RTNL flavour, however we can traverse it while
> holding only RCU, so add the RCU search. Add only one function that will be
> used further, other functions can be added easily afterwards, if anyone
> would need them.
[...]
> +static struct netdev_adjacent *__netdev_find_adj_rcu(struct net_device *dev,
> +						     struct net_device *adj_dev,
> +						     bool upper, bool neighbour)
> +{
> +	struct netdev_adjacent *adj;
> +	struct list_head *adj_list;
> +
> +	if (neighbour)
> +		adj_list = upper ? &dev->adj_list.upper :
> +				   &dev->adj_list.lower;
> +	else
> +		adj_list = upper ? &dev->all_adj_list.upper :
> +				   &dev->all_adj_list.lower;

I think it would be clearer to make adj_list a parameter, rather than
taking the dev and a pair of booleans.

Ben.

> +	list_for_each_entry_rcu(adj, adj_list, list) {
> +		if (adj->dev == adj_dev)
> +			return adj;
> +	}
> +	return NULL;
> +}
> +
> +static struct netdev_adjacent *__netdev_lower_find_rcu(struct net_device *dev,
> +							struct net_device *ldev)
> +{
> +	return __netdev_find_adj_rcu(dev, ldev, false, true);
> +}
> +
>  static struct netdev_adjacent *__netdev_find_adj(struct net_device *dev,
>  						 struct net_device *adj_dev,
>  						 bool upper, bool neighbour)

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply

* [PATCH net RFC 2/2] ixgbe: fix sleep bug caused by napi_disable inside local_bh_disable()d context
From: Jacob Keller @ 2013-09-17  0:59 UTC (permalink / raw)
  To: netdev
In-Reply-To: <20130917005703.13357.70276.stgit@jekeller-desk1.amr.corp.intel.com>

This patch fixes a bug caused by calling napi_disable after local_bh_disable.
It is possible for napi_disable to sleep, (though not guarunteed) so it could
cause an atomic sleep bug during the  schedule() call in msleep. This patch
resolves the issue by moving the local_bh_disable() calls inside the for loop
in ixgbe_napi_disable_all().

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Eliezer Tamir <eliezer.tamir@linux.intel.com>
Cc: Alexander Duyck <alexander.duyck@intel.com>
Cc: Hyong-Youb Kim <hykim@myri.com>
Cc: Amir Vadai <amirv@mellanox.com>
Cc: Dmitry Kravkov <dmitry@broadcom.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ce3eb60..39b3424 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -3891,15 +3891,15 @@ static void ixgbe_napi_disable_all(struct ixgbe_adapter *adapter)
 {
 	int q_idx;
 
-	local_bh_disable(); /* for ixgbe_qv_lock_napi() */
 	for (q_idx = 0; q_idx < adapter->num_q_vectors; q_idx++) {
 		napi_disable(&adapter->q_vector[q_idx]->napi);
+		local_bh_disable(); /* for ixgbe_qv_lock_napi() */
 		while (!ixgbe_qv_lock_napi(adapter->q_vector[q_idx])) {
 			pr_info("QV %d locked\n", q_idx);
 			mdelay(1);
 		}
+		local_bh_enable();
 	}
-	local_bh_enable();
 }
 
 #ifdef CONFIG_IXGBE_DCB

^ permalink raw reply related

* [PATCH net RFC 1/2] netdevice: add might_sleep() call to napi_disable
From: Jacob Keller @ 2013-09-17  0:59 UTC (permalink / raw)
  To: netdev
In-Reply-To: <20130917005703.13357.70276.stgit@jekeller-desk1.amr.corp.intel.com>

napi_disable potentially calls msleep(1) if the NAPI_STATE_SCHED bit is
previously set by something else. Because it does not always call msleep, it
was difficult to track down a bug related to calling napi_disable within
local_bh_disable()d context. This patch adds a might_sleep() call to the
napi_disable routine in order to aid in the future debugging of similar issues.
This will cause a BUG in drivers which have implemented busy polling in a
similar fashion to ixgbe, and which call napi_disable inside the
local_bh_disable()d section where the vector napi lock is taken.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Eliezer Tamir <eliezer.tamir@linux.intel.com>
Cc: Alex Duyck <alexander.h.duyck@intel.com>
Cc: Hyong-Youb Kim <hykim@myri.com>
Cc: Amir Vadai <amirv@mellanox.com>
Cc: Dmitry Kravkov <dmitry@broadcom.com>
---
 include/linux/netdevice.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 041b42a..5e3ef61 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -483,6 +483,8 @@ extern void napi_hash_del(struct napi_struct *napi);
  */
 static inline void napi_disable(struct napi_struct *n)
 {
+	might_sleep();
+
 	set_bit(NAPI_STATE_DISABLE, &n->state);
 	while (test_and_set_bit(NAPI_STATE_SCHED, &n->state))
 		msleep(1);

^ permalink raw reply related

* [PATCH net RFC 0/2] Series short description
From: Jacob Keller @ 2013-09-17  0:59 UTC (permalink / raw)
  To: netdev

This series contains patches which help resolve a 'sleeping function called
from invalid context' bug introduced by the busy polling code. The first patch
simply adds a might_sleep() call to napi_disable. The second patch provides the
actual fix for the sleeping during a local_bh_disable()d section of code. I
tried to Cc everyone that might be interested in the solution, as I am unsure
if my solution of moving the local_bh_disable() call is correct.

---

Jacob Keller (2):
      netdevice: add might_sleep() call to napi_disable
      ixgbe: fix sleep bug caused by napi_disable inside local_bh_disable()d context


 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 ++--
 include/linux/netdevice.h                     |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

--
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>

^ permalink raw reply

* Re: [PATCH net-next] net loopback: Set loopback_dev to NULL when freed
From: Eric Dumazet @ 2013-09-17  0:50 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: David S. Miller, Eric Dumazet, Jiri Pirko, Alexander Duyck,
	Cong Wang, netdev, Francesco Ruggeri
In-Reply-To: <87k3iggw8m.fsf_-_@xmission.com>

On Mon, 2013-09-16 at 16:52 -0700, Eric W. Biederman wrote:
> It has recently turned up that we have a number of long standing bugs
> in the network stack cleanup code with use of the loopback device
> after it has been freed that have not turned up because in most cases
> the storage allocated to the loopback device is not reused, when those
> accesses happen.
> 
> Set looback_dev to NULL to trigger oopses instead of silent data corrupt
> when we hit this class of bug.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---

Acked-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply

* [PATCH v3 net-next 27/27] net: create sysfs symlinks for neighbour devices
From: Veaceslav Falico @ 2013-09-17  0:46 UTC (permalink / raw)
  To: netdev
  Cc: jiri, Veaceslav Falico, Jay Vosburgh, Andy Gospodarek,
	David S. Miller, Eric Dumazet, Alexander Duyck
In-Reply-To: <1379378812-18346-1-git-send-email-vfalico@redhat.com>

Also, remove the same functionality from bonding - it will be already done
for any device that links to its lower/upper neighbour.

The links will be created for dev's kobject, and will look like
lower_eth0 for lower device eth0 and upper_bridge0 for upper device
bridge0.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jiri Pirko <jiri@resnulli.us>
CC: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

Notes:
    v2  -> v3:
    No change.
    
    v1  -> v2:
    No changes.
    
    RFC -> v1:
    Rename lower devices from slave_eth0 to lower_eth0, so that it meets
    the lower/upper naming. I've searched for any active users of bonding's
    slave_eth0, and seems that nobody's actively using it or relying on it.

 drivers/net/bonding/bond_main.c  | 11 +----------
 drivers/net/bonding/bond_sysfs.c | 25 -------------------------
 drivers/net/bonding/bonding.h    |  2 --
 net/core/dev.c                   | 29 +++++++++++++++++++++++++++++
 4 files changed, 30 insertions(+), 37 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 707b0bc..2c0e4a5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1612,15 +1612,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 	read_unlock(&bond->lock);
 
-	res = bond_create_slave_symlinks(bond_dev, slave_dev);
-	if (res)
-		goto err_detach;
-
 	res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
 					 new_slave);
 	if (res) {
 		pr_debug("Error %d calling netdev_rx_handler_register\n", res);
-		goto err_dest_symlinks;
+		goto err_detach;
 	}
 
 	res = bond_master_upper_dev_link(bond_dev, slave_dev, new_slave);
@@ -1642,9 +1638,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 err_unregister:
 	netdev_rx_handler_unregister(slave_dev);
 
-err_dest_symlinks:
-	bond_destroy_slave_symlinks(bond_dev, slave_dev);
-
 err_detach:
 	if (!USES_PRIMARY(bond->params.mode))
 		bond_hw_addr_flush(bond_dev, slave_dev);
@@ -1842,8 +1835,6 @@ static int __bond_release_one(struct net_device *bond_dev,
 			bond_dev->name, slave_dev->name, bond_dev->name);
 
 	/* must do this from outside any spinlocks */
-	bond_destroy_slave_symlinks(bond_dev, slave_dev);
-
 	vlan_vids_del_by_dev(slave_dev, bond_dev);
 
 	/* If the mode USES_PRIMARY, then this cases was handled above by
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index fd178a4..ef75471 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -168,31 +168,6 @@ static const struct class_attribute class_attr_bonding_masters = {
 	.namespace = bonding_namespace,
 };
 
-int bond_create_slave_symlinks(struct net_device *master,
-			       struct net_device *slave)
-{
-	char linkname[IFNAMSIZ+7];
-	int ret = 0;
-
-	/* create a link from the master to the slave */
-	sprintf(linkname, "slave_%s", slave->name);
-	ret = sysfs_create_link(&(master->dev.kobj), &(slave->dev.kobj),
-				linkname);
-
-	return ret;
-
-}
-
-void bond_destroy_slave_symlinks(struct net_device *master,
-				 struct net_device *slave)
-{
-	char linkname[IFNAMSIZ+7];
-
-	sprintf(linkname, "slave_%s", slave->name);
-	sysfs_remove_link(&(master->dev.kobj), linkname);
-}
-
-
 /*
  * Show the slaves in the current bond.
  */
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 0e8e00e..74efa45 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -410,8 +410,6 @@ int bond_create(struct net *net, const char *name);
 int bond_create_sysfs(struct bond_net *net);
 void bond_destroy_sysfs(struct bond_net *net);
 void bond_prepare_sysfs_group(struct bonding *bond);
-int bond_create_slave_symlinks(struct net_device *master, struct net_device *slave);
-void bond_destroy_slave_symlinks(struct net_device *master, struct net_device *slave);
 int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev);
 int bond_release(struct net_device *bond_dev, struct net_device *slave_dev);
 void bond_mii_monitor(struct work_struct *);
diff --git a/net/core/dev.c b/net/core/dev.c
index 27ca03f..5fe2dd0 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4638,6 +4638,7 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
 					bool upper, void *private)
 {
 	struct netdev_adjacent *adj, *neigh = NULL;
+	char linkname[IFNAMSIZ+7];
 	int ret;
 
 	adj = __netdev_find_adj(dev, adj_dev, upper, false);
@@ -4685,6 +4686,16 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
 			 */
 			if (master)
 				neigh->private = private;
+
+			sprintf(linkname, "lower_%s", adj_dev->name);
+			ret = sysfs_create_link(&(dev->dev.kobj),
+						&(adj_dev->dev.kobj),
+						linkname);
+			if (ret) {
+				kfree(neigh);
+				kfree(adj);
+				return ret;
+			}
 			list_add_tail_rcu(&neigh->list,
 					  &dev->adj_list.lower);
 		}
@@ -4692,10 +4703,24 @@ static int __netdev_adjacent_dev_insert(struct net_device *dev,
 		return 0;
 	}
 
+	/* it's upper neighbour, we don't care if it's master or not */
+	if (neigh) {
+		sprintf(linkname, "upper_%s", adj_dev->name);
+		ret = sysfs_create_link(&(dev->dev.kobj), &(adj_dev->dev.kobj),
+					linkname);
+		if (ret) {
+			kfree(neigh);
+			kfree(adj);
+			return ret;
+		}
+	}
+
 	/* Ensure that master upper link is always the first item in list. */
 	if (master) {
 		ret = sysfs_create_link(&(dev->dev.kobj), &(adj_dev->dev.kobj), "master");
 		if (ret) {
+			if (neigh)
+				sysfs_remove_link(&(dev->dev.kobj), linkname);
 			kfree(neigh);
 			kfree(adj);
 			return ret;
@@ -4736,6 +4761,7 @@ void __netdev_adjacent_dev_remove(struct net_device *dev,
 				  bool is_neigh)
 {
 	struct netdev_adjacent *adj, *neighbour;
+	char linkname[IFNAMSIZ+7];
 
 	if (upper) {
 		adj = __netdev_all_upper_find(dev, adj_dev);
@@ -4758,6 +4784,9 @@ void __netdev_adjacent_dev_remove(struct net_device *dev,
 			 adj_dev->name);
 		if (neighbour->master && upper)
 			sysfs_remove_link(&(dev->dev.kobj), "master");
+		sprintf(linkname, "%s_%s", upper ? "upper" : "lower",
+			adj_dev->name);
+		sysfs_remove_link(&(dev->dev.kobj), linkname);
 		list_del_rcu(&neighbour->list);
 		dev_put(adj_dev);
 		kfree_rcu(neighbour, rcu);
-- 
1.8.4

^ permalink raw reply related

* [PATCH v3 net-next 20/27] bonding: remove bond_prev_slave()
From: Veaceslav Falico @ 2013-09-17  0:46 UTC (permalink / raw)
  To: netdev; +Cc: jiri, Veaceslav Falico, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <1379378812-18346-1-git-send-email-vfalico@redhat.com>

We don't really need it, and it's really hard to RCUify the list->prev.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---

Notes:
    v2  -> v3:
    No change.
    
    v1  -> v2:
    No changes.
    
    RFC -> v1:
    New patch.

 drivers/net/bonding/bond_main.c | 9 +++------
 drivers/net/bonding/bonding.h   | 4 ----
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index c91b754..65eae12 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1255,7 +1255,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
 	const struct net_device_ops *slave_ops = slave_dev->netdev_ops;
-	struct slave *new_slave = NULL;
+	struct slave *new_slave = NULL, *prev_slave;
 	struct sockaddr addr;
 	int link_reporting;
 	int res = 0, i;
@@ -1472,6 +1472,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 	write_lock_bh(&bond->lock);
 
+	prev_slave = bond_last_slave(bond);
 	bond_attach_slave(bond, new_slave);
 
 	new_slave->delay = 0;
@@ -1566,9 +1567,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			 */
 			bond_3ad_initialize(bond, 1000/AD_TIMER_INTERVAL);
 		} else {
-			struct slave *prev_slave;
-
-			prev_slave = bond_prev_slave(bond, new_slave);
 			SLAVE_AD_INFO(new_slave).id =
 				SLAVE_AD_INFO(prev_slave).id + 1;
 		}
@@ -3506,9 +3504,8 @@ static int bond_change_mtu(struct net_device *bond_dev, int new_mtu)
 	 */
 
 	bond_for_each_slave(bond, slave, iter) {
-		pr_debug("s %p s->p %p c_m %p\n",
+		pr_debug("s %p c_m %p\n",
 			 slave,
-			 bond_prev_slave(bond, slave),
 			 slave->dev->netdev_ops->ndo_change_mtu);
 
 		res = dev_set_mtu(slave->dev, new_mtu);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index b82474d..03daadd 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -96,10 +96,6 @@
 	(bond_is_last_slave(bond, pos) ? bond_first_slave(bond) : \
 					 bond_to_slave((pos)->list.next))
 
-#define bond_prev_slave(bond, pos) \
-	(bond_is_first_slave(bond, pos) ? bond_last_slave(bond) : \
-					  bond_to_slave((pos)->list.prev))
-
 /**
  * bond_for_each_slave - iterate over all slaves
  * @bond:	the bond holding this list
-- 
1.8.4

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox