Netdev List
 help / color / mirror / Atom feed
* Re: [patch net-next 0/3] team: couple patches
From: David Miller @ 2012-04-21 20:27 UTC (permalink / raw)
  To: jpirko; +Cc: netdev, eric.dumazet
In-Reply-To: <1334932926-23996-1-git-send-email-jpirko@redhat.com>

From: Jiri Pirko <jpirko@redhat.com>
Date: Fri, 20 Apr 2012 16:42:03 +0200

> Jiri Pirko (3):
>   team: lb: let userspace care about port macs
>   team: allow to enable/disable ports
>   team: add per-port option for enabling/disabling ports

All applied, thanks Jiri.

^ permalink raw reply

* Re: [PATCH net-next] net: change big iov allocations
From: David Miller @ 2012-04-21 20:24 UTC (permalink / raw)
  To: eric.dumazet; +Cc: netdev, mikew
In-Reply-To: <1334945041.2395.243.camel@edumazet-glaptop>

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 20 Apr 2012 20:04:01 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> iov of more than 8 entries are allocated in sendmsg()/recvmsg() through
> sock_kmalloc()
> 
> As these allocations are temporary only and small enough, it makes sense
> to use plain kmalloc() and avoid sk_omem_alloc atomic overhead.
> 
> Slightly changed fast path to be even faster.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Mike Waychison <mikew@google.com>

Applied, thanks Eric.

^ permalink raw reply

* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: David Miller @ 2012-04-21 20:18 UTC (permalink / raw)
  To: john.r.fastabend
  Cc: therbert, alexander.duyck, alexander.h.duyck, jeffrey.t.kirsher,
	netdev, gospo, sassmann
In-Reply-To: <4F92DFF3.2080301@intel.com>

From: John Fastabend <john.r.fastabend@intel.com>
Date: Sat, 21 Apr 2012 09:27:31 -0700

> On 4/20/2012 11:01 PM, Tom Herbert wrote:
>>> I don't recall the exact reason now, as John said I think it had to do
>>> with the ethtool tests not using the same cleanup routine and leaving
>>> us in a bad state.  I am pretty sure there was some path in which
>>> where the call was didn't work but I do not recall the exact details
>>> now.  Most of the reason for moving it is due to the fact that the
>>> reset is now also clearing the bit, and from the driver perspective we
>>> didn't need it in two places.  After looking it all over again, I
>>> suppose this causes a cosmetic issue for the bql "inflight" statistic
>>> in sysfs since the value will be retained until the interface is
>>> brought back up with this change.  Is that an issue or something that
>>> can be lived with since the interface isn't active anyway in this
>>> case?
>>>
>> On the surface, it seems cleaner and probably a simpler convention to
>> clear the state when the buffers are being freed.  If there's a good
>> reason not to do this for igb, it makes me wonder if this should be
>> done the same way in other drivers...
>> 
>> Tom
> 
> Just dug up the old thread. This was to fix a bug that byte queue limits
> was causing with some of the loopback tests evoked from ethtool. The
> loopback test uses a separate path to free the buffers which wasn't
> calling netdev_tx_reset_queue().
> 
> We should likely just explicitly clear the state in each routine rather
> than try to be clever IMO.

Agreed.

^ permalink raw reply

* Re: net_sched: gred: red_calc_qavg() called with current qavg for backlog?
From: David Miller @ 2012-04-21 20:10 UTC (permalink / raw)
  To: david.ward; +Cc: tgraf, eric.dumazet, netdev
In-Reply-To: <4F930087.7010004@ll.mit.edu>

From: "Ward, David - 0663 - MITLL" <david.ward@ll.mit.edu>
Date: Sat, 21 Apr 2012 14:46:31 -0400

> In net/sched/sch_gred.c:
> 
> static int gred_dump(struct Qdisc *sch, struct sk_buff *skb)
> {
>      struct gred_sched *table = qdisc_priv(sch);
> ...
>      for (i = 0; i < MAX_DPs; i++) {
>          struct gred_sched_data *q = table->tab[i];
>          struct tc_gred_qopt opt;
> ...
>          opt.qave = red_calc_qavg(&q->parms, &q->vars, q->vars.qavg);
> 
> 
> I can't tell if red_calc_qavg is intentionally being passed the current 
> qavg as the backlog (which effectively causes qavg to only be 
> re-calculated if we are idling)?  Or should this be:
> 
>          opt.qave = red_calc_qavg(&q->parms,
>                                   &q->vars,
>                                   gred_backlog(table, q, sch));

Looking at commit 22b33429ab93155895854e9518a253680a920493
("[PKT_SCHED]: GRED: Use new generic red interface") it appears
that this line is intentional so that the dump reports the same
qave as it would have before Thomas's changes.

Thomas?

^ permalink raw reply

* Re: [PATCH] ks8851: Fix request_irq/free_irq mismatch
From: David Miller @ 2012-04-21 19:56 UTC (permalink / raw)
  To: sboyd; +Cc: mjr, ben, netdev
In-Reply-To: <4F9055DF.9050509@codeaurora.org>

From: Stephen Boyd <sboyd@codeaurora.org>
Date: Thu, 19 Apr 2012 11:13:51 -0700

> On 04/19/12 10:17, mjr@cs.wisc.edu wrote:
>> From: Matt Renzelmann <mjr@cs.wisc.edu>
>>
>> The dev_id parameter passed to free_irq needs to match the one passed
>> to the corresponding request_irq.
>>
>> Signed-off-by: Matt Renzelmann <mjr@cs.wisc.edu>
> 
> Acked-by: Stephen Boyd <sboyd@codeaurora.org>

Applied, thanks.

^ permalink raw reply

* Re: [PATCH net-next 0/6] TCP connection repair (v4)
From: David Miller @ 2012-04-21 19:53 UTC (permalink / raw)
  To: xemul; +Cc: netdev
In-Reply-To: <4F901572.4040009@parallels.com>

From: Pavel Emelyanov <xemul@parallels.com>
Date: Thu, 19 Apr 2012 17:38:58 +0400

> Attempt #4 with an API for TCP connection recreation (previous one is
> at http://lists.openwall.net/netdev/2012/03/28/84) re-based on the
> today's net-next tree.
> 
> Changes since v3:
> 
> * Added repair for TCP options negotiated during 3WHS process, pointed
>   out by Li Yu. The explanation of how this happens is in patch #6.
> 
> * Named constant for sk_reuse values as proposed by Ben Hutching.
> 
> * Off-by-one in repair-queue sockoption caught by Ben.

All applied to net-next, nice work.

Please make the following fix for me.  The option recovery code will
result in unaligned accesses, for example you'll do a byte aligned
get_user() for the u16 MSS object in many cases, and this will trap
and cpus such as sparc.

Either add a padding facility or pass more structured data into the
socket option.

Thanks.

^ permalink raw reply

* Re: [PATCH] net/hyperv: Adding cancellation to ensure rndis filter is closed
From: David Miller @ 2012-04-21 19:38 UTC (permalink / raw)
  To: haiyangz; +Cc: wenqi_ma, netdev, kys
In-Reply-To: <A1F3067C9B68744AA19F6802BAB8FFDC0DD33BA8@TK5EX14MBXC221.redmond.corp.microsoft.com>

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Thu, 19 Apr 2012 15:18:41 +0000

>> From: Wenqi Ma [mailto:wenqi_ma@trendmicro.com.cn]
>> Sent: Thursday, April 19, 2012 6:40 AM
>> To: netdev@vger.kernel.org
>> Cc: davem@davemloft.net; Haiyang Zhang; Wenqi Ma
>> Subject: [PATCH] net/hyperv: Adding cancellation to ensure rndis filter is
>> closed
>> 
>> Although the network interface is down, the RX packets number which
>> could be observed by ifconfig may keep on increasing.
>> 
>> This is because the WORK scheduled in netvsc_set_multicast_list()
>> may be executed after netvsc_close(). That means the rndis filter
>> may be re-enabled by do_set_multicast() even if it was closed by
>> netvsc_close().
>> 
>> By canceling possible WORK before close the rndis filter, the issue
>> could be never happened.
>> 
>> Signed-off-by: Wenqi Ma <wenqi_ma@trendmicro.com.cn>
>> Reviewed-by: Wei Yongjun <yongjun_wei@trendmicro.com.cn>
 ...
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

Applied.

^ permalink raw reply

* Re: [RFC v4] Add TCP encap_rcv hook (repost)
From: David Miller @ 2012-04-21 19:37 UTC (permalink / raw)
  To: horms-/R6kz+dDXgpPR4JQBCEnsQ
  Cc: dev-yBygre7rU0TnMu66kgdUjQ, netdev-u79uwXL29TY76Z2rM5mHXA,
	eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
In-Reply-To: <20120419045333.GA21311-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>

From: Simon Horman <horms-/R6kz+dDXgpPR4JQBCEnsQ@public.gmane.org>
Date: Thu, 19 Apr 2012 13:53:35 +0900

> This hook is based on a hook of the same name provided by UDP.  It provides
> a way for to receive packets that have a TCP header and treat them in some
> alternate way.
> 
> It is intended to be used by an implementation of the STT tunneling
> protocol within Open vSwtich's datapath. A prototype of such an
> implementation has been made.
> 
> The STT draft is available at
> http://tools.ietf.org/html/draft-davie-stt-01

I think that unlike UDP, you need to let the stack handle the TCP
packet before taking it into your subsystem.  The reason is that
otherwise you'll need to handle packet ordering, sequence number gaps,
and proper TCP ACK'ing and timeout all inside of your module and
that's simply unacceptable.

Do what the SunRPC layer does, register a TCP socket for the port,
and use the ->data_ready() socket callback to consume the packets
into your subsystem.

That allows TCP to do all of it's work, and you just get a sane
in-order validated datastream on your end.

^ permalink raw reply

* Re: [PATCH 2/2] ks8851: Fix mutex deadlock in ks8851_net_stop()
From: David Miller @ 2012-04-21 19:34 UTC (permalink / raw)
  To: sboyd; +Cc: linux-kernel, netdev, tj, ben-linux
In-Reply-To: <1334805958-29119-2-git-send-email-sboyd@codeaurora.org>

From: Stephen Boyd <sboyd@codeaurora.org>
Date: Wed, 18 Apr 2012 20:25:58 -0700

> There is a potential deadlock scenario when the ks8851 driver
> is removed. The interrupt handler schedules a workqueue which
> acquires a mutex that ks8851_net_stop() also acquires before
> flushing the workqueue. Previously lockdep wouldn't be able
> to find this problem but now that it has the support we can
> trigger this lockdep warning by rmmoding the driver after
> an ifconfig up.
> 
> Fix the possible deadlock by disabling the interrupts in 
> the chip and then release the lock across the workqueue
> flushing. The mutex is only there to proect the registers
> anyway so this should be ok.
 ...
> Cc: Ben Dooks <ben-linux@fluff.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>

Applied.

^ permalink raw reply

* Re: [PATCH v5 2/3] netdev/of/phy: Add MDIO bus multiplexer support.
From: David Miller @ 2012-04-21 19:32 UTC (permalink / raw)
  To: ddaney.cavm
  Cc: grant.likely, rob.herring, devicetree-discuss, netdev,
	linux-kernel, linux-mips, afleming, galak, david.daney
In-Reply-To: <1334791254-15987-3-git-send-email-ddaney.cavm@gmail.com>

From: David Daney <ddaney.cavm@gmail.com>
Date: Wed, 18 Apr 2012 16:20:53 -0700

> +config MDIO_BUS_MUX
> +	tristate
> +	help
> +	  This module provides a driver framework for MDIO bus
> +	  multiplexers which connect one of several child MDIO busses
> +	  to a parent bus.  Switching between child busses is done by
> +	  device specific drivers.
> +

This driver uses OF and OF_MDIO, and therefore need dependencies upon
them.  Otherwise it can be enabled in configurations which will result
in build failures.

^ permalink raw reply

* Re: [PATCH net-next 0/9] pch_gbe: ptp bug fixes
From: David Miller @ 2012-04-21 19:29 UTC (permalink / raw)
  To: richardcochran; +Cc: netdev, tshimizu818
In-Reply-To: <cover.1334983471.git.richardcochran@gmail.com>

From: Richard Cochran <richardcochran@gmail.com>
Date: Sat, 21 Apr 2012 06:50:27 +0200

> The content in this patch series originally was posted to netdev by
> Takahiro on March 23, 2012, as a loose group of five patches. These
> patches addressed problems pointed out in review, both on and off
> list.
> 
> I have taken his work and rebased, squashed, and split it into the
> present form, and I also wrote commit messages that more fully explain
> the changes. Also, I added the last two patches myself.

All applied, thanks.

^ permalink raw reply

* Re: [PATCH 0/8] drivers/net: randconfig patches for kernel 3.3
From: David Miller @ 2012-04-21 19:29 UTC (permalink / raw)
  To: mathieu.poirier; +Cc: netdev, linux-kernel, arnd
In-Reply-To: <1334955376-17793-1-git-send-email-mathieu.poirier@linaro.org>

From: mathieu.poirier@linaro.org
Date: Fri, 20 Apr 2012 14:56:08 -0600

> From: Arnd Bergmann <arnd@arndb.de>
> 
> These patches fix miscellaneous problems when working
> with make randconfig.  They were discovered on kernel
> 3.1-rc4 and have been reformatted for 3.3.

All applied to net-next, thanks.

^ permalink raw reply

* Re: use-after-free in usbnet
From: David Miller @ 2012-04-21 19:23 UTC (permalink / raw)
  To: tom.leiming
  Cc: huajun.li.lee, oneukum, stern, davej, netdev, linux-usb,
	kernel-team
In-Reply-To: <CACVXFVOc0XZ+eLHGiVwKuiUResRk8Cj9MS4EPMx7k57a0tEJhA@mail.gmail.com>

From: Ming Lei <tom.leiming@gmail.com>
Date: Sat, 21 Apr 2012 09:49:51 +0800

> I see the problem, so looks skb_queue_walk_safe is not safe.
> I don' know why the 2nd ' tmp = skb->next' in  skb_queue_walk_safe
> is needed and it may become unsafe if skb is freed during current loop.

I can't see what the problem is, skb_queue_walk_safe() is perfect
and does exactly what it advertises to do.

If 'skb' is unlinked inside of an skb_queue_walk_safe() loop, that's
fine, because we won't touch 'skb' in the loop iteration tail code.

Instead, before the loop contents, we pre-fetch skb->next into 'tmp'
and then at the end we move 'skb' forward by simply assigning 'tmp'.

^ permalink raw reply

* net_sched: gred: red_calc_qavg() called with current qavg for backlog?
From: Ward, David - 0663 - MITLL @ 2012-04-21 18:46 UTC (permalink / raw)
  To: Thomas Graf, Eric Dumazet; +Cc: netdev@vger.kernel.org

In net/sched/sch_gred.c:

static int gred_dump(struct Qdisc *sch, struct sk_buff *skb)
{
     struct gred_sched *table = qdisc_priv(sch);
...
     for (i = 0; i < MAX_DPs; i++) {
         struct gred_sched_data *q = table->tab[i];
         struct tc_gred_qopt opt;
...
         opt.qave = red_calc_qavg(&q->parms, &q->vars, q->vars.qavg);


I can't tell if red_calc_qavg is intentionally being passed the current 
qavg as the backlog (which effectively causes qavg to only be 
re-calculated if we are idling)?  Or should this be:

         opt.qave = red_calc_qavg(&q->parms,
                                  &q->vars,
                                  gred_backlog(table, q, sch));

Thanks,

David

^ permalink raw reply

* Re: [PATCH 00/16] Swap-over-NBD without deadlocking V9
From: Eric B Munson @ 2012-04-21 18:15 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Andrew Morton, Linux-MM, Linux-Netdev, LKML, David Miller,
	Neil Brown, Peter Zijlstra, Mike Christie
In-Reply-To: <1334578624-23257-1-git-send-email-mgorman@suse.de>

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

On Mon, 16 Apr 2012, Mel Gorman wrote:

> Changelog since V8
>   o Rebase to 3.4-rc2
>   o Use page flag instead of slab fields to keep structures the same size
>   o Properly detect allocations from softirq context that use PF_MEMALLOC
>   o Ensure kswapd does not sleep while processes are throttled
>   o Do not accidentally throttle !_GFP_FS processes indefinitely
> 
> Changelog since V7
>   o Rebase to 3.3-rc2
>   o Take greater care propagating page->pfmemalloc to skb
>   o Propagate pfmemalloc from netdev_alloc_page to skb where possible
>   o Release RCU lock properly on preempt kernel
> 
> Changelog since V6
>   o Rebase to 3.1-rc8
>   o Use wake_up instead of wake_up_interruptible()
>   o Do not throttle kernel threads
>   o Avoid a potential race between kswapd going to sleep and processes being
>     throttled
> 
> Changelog since V5
>   o Rebase to 3.1-rc5
> 
> Changelog since V4
>   o Update comment clarifying what protocols can be used		(Michal)
>   o Rebase to 3.0-rc3
> 
> Changelog since V3
>   o Propogate pfmemalloc from packet fragment pages to skb		(Neil)
>   o Rebase to 3.0-rc2
> 
> Changelog since V2
>   o Document that __GFP_NOMEMALLOC overrides __GFP_MEMALLOC		(Neil)
>   o Use wait_event_interruptible					(Neil)
>   o Use !! when casting to bool to avoid any possibilitity of type
>     truncation								(Neil)
>   o Nicer logic when using skb_pfmemalloc_protocol			(Neil)
> 
> Changelog since V1
>   o Rebase on top of mmotm
>   o Use atomic_t for memalloc_socks		(David Miller)
>   o Remove use of sk_memalloc_socks in vmscan	(Neil Brown)
>   o Check throttle within prepare_to_wait	(Neil Brown)
>   o Add statistics on throttling instead of printk
> 
> When a user or administrator requires swap for their application, they
> create a swap partition and file, format it with mkswap and activate it
> with swapon. Swap over the network is considered as an option in diskless
> systems. The two likely scenarios are when blade servers are used as part
> of a cluster where the form factor or maintenance costs do not allow the
> use of disks and thin clients.
> 
> The Linux Terminal Server Project recommends the use of the
> Network Block Device (NBD) for swap according to the manual at
> https://sourceforge.net/projects/ltsp/files/Docs-Admin-Guide/LTSPManual.pdf/download
> There is also documentation and tutorials on how to setup swap over NBD
> at places like https://help.ubuntu.com/community/UbuntuLTSP/EnableNBDSWAP
> The nbd-client also documents the use of NBD as swap. Despite this, the
> fact is that a machine using NBD for swap can deadlock within minutes if
> swap is used intensively. This patch series addresses the problem.
> 
> The core issue is that network block devices do not use mempools like
> normal block devices do. As the host cannot control where they receive
> packets from, they cannot reliably work out in advance how much memory
> they might need. Some years ago, Peter Ziljstra developed a series of
> patches that supported swap over an NFS that at least one distribution
> is carrying within their kernels. This patch series borrows very heavily
> from Peter's work to support swapping over NBD as a pre-requisite to
> supporting swap-over-NFS. The bulk of the complexity is concerned with
> preserving memory that is allocated from the PFMEMALLOC reserves for use
> by the network layer which is needed for both NBD and NFS.
> 
> Patch 1 serialises access to min_free_kbytes. It's not strictly needed
> 	by this series but as the series cares about watermarks in
> 	general, it's a harmless fix. It could be merged independently
> 	and may be if CMA is merged in advance.
> 
> Patch 2 adds knowledge of the PFMEMALLOC reserves to SLAB and SLUB to
> 	preserve access to pages allocated under low memory situations
> 	to callers that are freeing memory.
> 
> Patch 3 introduces __GFP_MEMALLOC to allow access to the PFMEMALLOC
> 	reserves without setting PFMEMALLOC.
> 
> Patch 4 opens the possibility for softirqs to use PFMEMALLOC reserves
> 	for later use by network packet processing.
> 
> Patch 5 ignores memory policies when ALLOC_NO_WATERMARKS is set.
> 
> Patches 6-13 allows network processing to use PFMEMALLOC reserves when
> 	the socket has been marked as being used by the VM to clean pages. If
> 	packets are received and stored in pages that were allocated under
> 	low-memory situations and are unrelated to the VM, the packets
> 	are dropped.
> 
> 	Patch 11 reintroduces __netdev_alloc_page which the networking
> 	folk may object to but is needed in some cases to propogate
> 	pfmemalloc from a newly allocated page to an skb. If there is a
> 	strong objection, this patch can be dropped with the impact being
> 	that swap-over-network will be slower in some cases but it should
> 	not fail.
> 
> Patch 13 is a micro-optimisation to avoid a function call in the
> 	common case.
> 
> Patch 14 tags NBD sockets as being SOCK_MEMALLOC so they can use
> 	PFMEMALLOC if necessary.
> 
> Patch 15 notes that it is still possible for the PFMEMALLOC reserve
> 	to be depleted. To prevent this, direct reclaimers get throttled on
> 	a waitqueue if 50% of the PFMEMALLOC reserves are depleted.  It is
> 	expected that kswapd and the direct reclaimers already running
> 	will clean enough pages for the low watermark to be reached and
> 	the throttled processes are woken up.
> 
> Patch 16 adds a statistic to track how often processes get throttled
> 
> Some basic performance testing was run using kernel builds, netperf
> on loopback for UDP and TCP, hackbench (pipes and sockets), iozone
> and sysbench. Each of them were expected to use the sl*b allocators
> reasonably heavily but there did not appear to be significant
> performance variances.
> 
> For testing swap-over-NBD, a machine was booted with 2G of RAM with a
> swapfile backed by NBD. 8*NUM_CPU processes were started that create
> anonymous memory mappings and read them linearly in a loop. The total
> size of the mappings were 4*PHYSICAL_MEMORY to use swap heavily under
> memory pressure.
> 
> Without the patches and using SLUB, the machine locks up within minutes and
> runs to completion with them applied. With SLAB, the story is different
> as an unpatched kernel run to completion. However, the patched kernel
> completed the test 40% faster.
> 
>                                          3.4.0-rc2     3.4.0-rc2
>                                       vanilla-slab     swapnbd
> Sys Time Running Test (seconds)              87.90     73.45
> User+Sys Time Running Test (seconds)         91.93     76.91
> Total Elapsed Time (seconds)               4174.37   2953.96
> 

I have tested these with an artificial swap benchmark and with a large project
compile on a beagle board.  They work great for me.  My tests only used this
set via swap over NFS so it probably wasn't very thorough coverage.

Tested-by: Eric B Munson <emunson@mgebm.net>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH 00/11] Swap-over-NFS without deadlocking V3
From: Eric B Munson @ 2012-04-21 18:13 UTC (permalink / raw)
  To: Mel Gorman
  Cc: Andrew Morton, Linux-MM, Linux-Netdev, Linux-NFS, LKML,
	David Miller, Trond Myklebust, Neil Brown, Christoph Hellwig,
	Peter Zijlstra, Mike Christie
In-Reply-To: <1334578675-23445-1-git-send-email-mgorman@suse.de>

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

On Mon, 16 Apr 2012, Mel Gorman wrote:

> Changelog since V2
> o Nothing significant, just rebases. A radix tree lookup is replaced with
>   a linear search would be the biggest rebase artifact
> 
> This patch series is based on top of "Swap-over-NBD without deadlocking v9"
> as it depends on the same reservation of PF_MEMALLOC reserves logic.
> 
> When a user or administrator requires swap for their application, they
> create a swap partition and file, format it with mkswap and activate it with
> swapon. In diskless systems this is not an option so if swap if required
> then swapping over the network is considered.  The two likely scenarios
> are when blade servers are used as part of a cluster where the form factor
> or maintenance costs do not allow the use of disks and thin clients.
> 
> The Linux Terminal Server Project recommends the use of the Network
> Block Device (NBD) for swap but this is not always an option.  There is
> no guarantee that the network attached storage (NAS) device is running
> Linux or supports NBD. However, it is likely that it supports NFS so there
> are users that want support for swapping over NFS despite any performance
> concern. Some distributions currently carry patches that support swapping
> over NFS but it would be preferable to support it in the mainline kernel.
> 
> Patch 1 avoids a stream-specific deadlock that potentially affects TCP.
> 
> Patch 2 is a small modification to SELinux to avoid using PFMEMALLOC
> 	reserves.
> 
> Patch 3 adds three helpers for filesystems to handle swap cache pages.
> 	For example, page_file_mapping() returns page->mapping for
> 	file-backed pages and the address_space of the underlying
> 	swap file for swap cache pages.
> 
> Patch 4 adds two address_space_operations to allow a filesystem
> 	to pin all metadata relevant to a swapfile in memory. Upon
> 	successful activation, the swapfile is marked SWP_FILE and
> 	the address space operation ->direct_IO is used for writing
> 	and ->readpage for reading in swap pages.
> 
> Patch 5 notes that patch 3 is bolting
> 	filesystem-specific-swapfile-support onto the side and that
> 	the default handlers have different information to what
> 	is available to the filesystem. This patch refactors the
> 	code so that there are generic handlers for each of the new
> 	address_space operations.
> 
> Patch 6 adds an API to allow a vector of kernel addresses to be
> 	translated to struct pages and pinned for IO.
> 
> Patch 7 updates NFS to use the helpers from patch 3 where necessary.
> 
> Patch 8 avoids setting PF_private on PG_swapcache pages within NFS.
> 
> Patch 9 implements the new swapfile-related address_space operations
> 	for NFS and teaches the direct IO handler how to manage
> 	kernel addresses.
> 
> Patch 10 prevents page allocator recursions in NFS by using GFP_NOIO
> 	where appropriate.
> 
> Patch 11 fixes a NULL pointer dereference that occurs when using
> 	swap-over-NFS.
> 
> With the patches applied, it is possible to mount a swapfile that is on an
> NFS filesystem. Swap performance is not great with a swap stress test taking
> roughly twice as long to complete than if the swap device was backed by NBD.
> 


FWIW, I'd like to see these go in, I use them for giving ARM boards with NFS
root file systems swap space.

I have tested these with an artificial swap benchmark and with a large project
compile on a beagle board.  They work great for me.

Tested-by: Eric B Munson <emunson@mgebm.net>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Proposition
From: Li Hao @ 2012-04-21 17:55 UTC (permalink / raw)
  To: netdev


Good day,

I am Mr. Li Hao, Chief Financial Officer of China Merchants Bank, P.R.C. I have a discreet proposal for you in the tune of One Hundred & Five Million EUR, after successful transfer, we shall share in the ratio of forty for you and sixty for me. Please reply for specifics.

Yours,
Mr. Li Hao.

^ permalink raw reply

* WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
From: Alex Villací­s Lasso @ 2012-04-21 17:21 UTC (permalink / raw)
  To: netdev

I am getting the following WARNING while running 3.4-rc3 x86_64. Every 
time I get this, I am running my Bittorrent client (transmission-gtk).

[ 1494.720011] ------------[ cut here ]------------
[ 1494.720024] WARNING: at net/sched/sch_generic.c:256 
dev_watchdog+0x277/0x280()
[ 1494.720027] Hardware name: OEM
[ 1494.720031] NETDEV WATCHDOG: p17p1 (r8169): transmit queue 0 timed out
[ 1494.720034] Modules linked in: fuse vboxpci(O) vboxnetadp(O) 
vboxnetflt(O) lockd vboxdrv(O) nf_conntrack_ipv4 nf_defrag_ipv4 
ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 xt_state nf_conntrack 
ip6table_filter ip6_tables snd_hda_codec_realtek snd_hda_intel 
snd_hda_codec snd_hwdep snd_seq snd_seq_device ppdev snd_pcm uinput 
coretemp microcode pcspkr iTCO_wdt iTCO_vendor_support i2c_i801 
parport_pc parport snd_timer snd soundcore r8169 mii snd_page_alloc 
sunrpc binfmt_misc floppy i915 drm_kms_helper drm i2c_algo_bit i2c_core 
video [last unloaded: scsi_wait_scan]
[ 1494.720100] Pid: 0, comm: swapper/1 Tainted: G           O 3.4.0-rc3 #35
[ 1494.720103] Call Trace:
[ 1494.720106] <IRQ>  [<ffffffff81059bbf>] warn_slowpath_common+0x7f/0xc0
[ 1494.720120]  [<ffffffff81059cb6>] warn_slowpath_fmt+0x46/0x50
[ 1494.720127]  [<ffffffff8151ab97>] dev_watchdog+0x277/0x280
[ 1494.720134]  [<ffffffff81068f61>] run_timer_softirq+0x131/0x440
[ 1494.720139]  [<ffffffff8151a920>] ? qdisc_reset+0x50/0x50
[ 1494.720145]  [<ffffffff810611a8>] __do_softirq+0xb8/0x280
[ 1494.720151]  [<ffffffff8101cd62>] ? native_sched_clock+0x22/0x80
[ 1494.720157]  [<ffffffff81090895>] ? sched_clock_local+0x25/0x90
[ 1494.720164]  [<ffffffff8161619c>] call_softirq+0x1c/0x30
[ 1494.720170]  [<ffffffff810172e5>] do_softirq+0x65/0xa0
[ 1494.720174]  [<ffffffff8106169e>] irq_exit+0x9e/0xc0
[ 1494.720180]  [<ffffffff81616acb>] smp_apic_timer_interrupt+0x6b/0x98
[ 1494.720186]  [<ffffffff8161584a>] apic_timer_interrupt+0x6a/0x70
[ 1494.720190] <EOI>  [<ffffffff8101e253>] ? mwait_idle+0x93/0x320
[ 1494.720199]  [<ffffffff8101e1fa>] ? mwait_idle+0x3a/0x320
[ 1494.720204]  [<ffffffff8101ee19>] cpu_idle+0xd9/0x130
[ 1494.720210]  [<ffffffff815fb7cf>] start_secondary+0x266/0x26d
[ 1494.720216] ---[ end trace b0a1361739c69d32 ]---
[ 1494.721181] r8169 0000:02:00.0: p17p1: link up

^ permalink raw reply

* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: John Fastabend @ 2012-04-21 16:27 UTC (permalink / raw)
  To: Tom Herbert
  Cc: Alexander Duyck, Alexander Duyck, Jeff Kirsher, davem, netdev,
	gospo, sassmann
In-Reply-To: <CA+mtBx-TJAr-cu-vKYq6m2ZQzn1X1vf+_PKUZXDirnKEF67EAw@mail.gmail.com>

On 4/20/2012 11:01 PM, Tom Herbert wrote:
>> I don't recall the exact reason now, as John said I think it had to do
>> with the ethtool tests not using the same cleanup routine and leaving
>> us in a bad state.  I am pretty sure there was some path in which
>> where the call was didn't work but I do not recall the exact details
>> now.  Most of the reason for moving it is due to the fact that the
>> reset is now also clearing the bit, and from the driver perspective we
>> didn't need it in two places.  After looking it all over again, I
>> suppose this causes a cosmetic issue for the bql "inflight" statistic
>> in sysfs since the value will be retained until the interface is
>> brought back up with this change.  Is that an issue or something that
>> can be lived with since the interface isn't active anyway in this
>> case?
>>
> On the surface, it seems cleaner and probably a simpler convention to
> clear the state when the buffers are being freed.  If there's a good
> reason not to do this for igb, it makes me wonder if this should be
> done the same way in other drivers...
> 
> Tom

Just dug up the old thread. This was to fix a bug that byte queue limits
was causing with some of the loopback tests evoked from ethtool. The
loopback test uses a separate path to free the buffers which wasn't
calling netdev_tx_reset_queue().

We should likely just explicitly clear the state in each routine rather
than try to be clever IMO.

.John

^ permalink raw reply

* My e1000e GBE card is eating all port 623 pks
From: Joakim Tjernlund @ 2012-04-21 13:31 UTC (permalink / raw)
  To: netdev


Looks like port 623 is some mgmt protocol and our e1000e boards are eating these
pkgs and this trips NIS, finger and yptest hangs for a long time before timing out and
moving on.

Is there a way to tell the network stack not to eat port 623 pkgs or
have NIS not to use port 623?

 Jocke

^ permalink raw reply

* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21  9:40 UTC (permalink / raw)
  To: Huajun Li
  Cc: Oliver Neukum, Alan Stern, Dave Jones,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	Fedora Kernel Team
In-Reply-To: <CA+v9cxZRpthaZ=64tLzKf=AGOqaRVwe5o0UMadiXGuiuXiA7uA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi Huajun,

On Sat, Apr 21, 2012 at 4:23 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hi Huajun,
>>
>> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> Did we on the same page, could you please review my patch again?
>>>
>>> My draft patch was based on current mainline( 3.4.0-rc3)  which had
>>> already integrated your previous patch. And in my patch, it replaced
>>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
>>> = skb->next'  any more.
>>
>> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
>> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
>> the oops, does it?
>>
>
> No.
> In each loop, my patch traverse the queue from its head, and it always
> holds  q->lock when it need refer "skb->next", this can make sure the
> right skb is not moved out of rxq/txq.

OK, your patch can avoid the oops, sorry for miss the point.

>
> Can this fix what you concern? If so, IMO, there is no need to revert
> your previous patch.

But your patch may introduce another problem, in fact, what your patch does
is basically same with the below change[1]:

So we can find easily that one same URB may be unlinked more than one
time with your patch because usb_unlink_urb is asynchronous even though
it behaves synchronously sometimes.

I remembered that is not allowed, at least usb_unlink_urb's comment says so:

          URBs complete only once per submission, and may be canceled only
          once per submission.


[1], against 3.4.0-rc3
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..aadf009 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -578,15 +578,19 @@ EXPORT_SYMBOL_GPL(usbnet_purge_paused_rxq);
 static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
 {
 	unsigned long		flags;
-	struct sk_buff		*skb, *skbnext;
+	struct sk_buff		*skb;
 	int			count = 0;

 	spin_lock_irqsave (&q->lock, flags);
-	skb_queue_walk_safe(q, skb, skbnext) {
+	while (1) {
 		struct skb_data		*entry;
 		struct urb		*urb;
 		int			retval;

+		skb = q->next;
+		if (skb == (struct sk_buff *)q)
+			break;
+
 		entry = (struct skb_data *) skb->cb;
 		urb = entry->urb;


Thanks,
-- 
Ming Lei
--
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 related

* Re: use-after-free in usbnet
From: Huajun Li @ 2012-04-21  8:23 UTC (permalink / raw)
  To: Ming Lei
  Cc: Oliver Neukum, Alan Stern, Dave Jones,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	Fedora Kernel Team
In-Reply-To: <CACVXFVOMTbq0zbmsZAt-Pyc=3oqQ=UcWV5HgNryu7s6oMhKpQg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Sat, Apr 21, 2012 at 3:56 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Huajun,
>
> On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Did we on the same page, could you please review my patch again?
>>
>> My draft patch was based on current mainline( 3.4.0-rc3)  which had
>> already integrated your previous patch. And in my patch, it replaced
>> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
>> = skb->next'  any more.
>
> Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
> the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
> the oops, does it?
>

No.
In each loop, my patch traverse the queue from its head, and it always
holds  q->lock when it need refer "skb->next", this can make sure the
right skb is not moved out of rxq/txq.

Can this fix what you concern? If so, IMO, there is no need to revert
your previous patch.
--
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: use-after-free in usbnet
From: Huajun Li @ 2012-04-21  8:00 UTC (permalink / raw)
  To: Ming Lei
  Cc: Oliver Neukum, Alan Stern, Dave Jones,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	Fedora Kernel Team
In-Reply-To: <CACVXFVNc_S8pTaBqMzQZx6Dt-tSP_9iXepxJzv=iR9BFu=Tj8g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Sat, Apr 21, 2012 at 3:45 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Huajun,
>
> On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Just skip trying this per your following email's comments.
>>
>> I will prepare a new patch later, if you'd like to try it.
>
> The below patch reverts the below commits:
>
>       0956a8c20b23d429e79ff86d4325583fc06f9eb4
>      (usbnet: increase URB reference count before usb_unlink_urb)
>
>      4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
>      (net/usbnet: avoid recursive locking in usbnet_stop())
>
> and keep holding tx/rx queue lock during unlinking, but avoid
> to acquire the same queue lock inside complete handler triggered by
> usb_unlink_urb.
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..effb34e 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -291,9 +291,11 @@ static void defer_bh(struct usbnet *dev, struct
> sk_buff *skb, struct sk_buff_hea
>  {
>        unsigned long           flags;
>
> -       spin_lock_irqsave(&list->lock, flags);
> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
> +               spin_lock_irqsave(&list->lock, flags);
>        __skb_unlink(skb, list);
> -       spin_unlock(&list->lock);
> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
> +               spin_unlock(&list->lock);
>        spin_lock(&dev->done.lock);
>        __skb_queue_tail(&dev->done, skb);
>        if (dev->done.qlen == 1)
> @@ -345,7 +347,8 @@ static int rx_submit (struct usbnet *dev, struct
> urb *urb, gfp_t flags)
>        usb_fill_bulk_urb (urb, dev->udev, dev->in,
>                skb->data, size, rx_complete, skb);
>
> -       spin_lock_irqsave (&dev->rxq.lock, lockflags);
> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
> +               spin_lock_irqsave (&dev->rxq.lock, lockflags);
>
>        if (netif_running (dev->net) &&
>            netif_device_present (dev->net) &&
> @@ -377,7 +380,8 @@ static int rx_submit (struct usbnet *dev, struct
> urb *urb, gfp_t flags)
>                netif_dbg(dev, ifdown, dev->net, "rx: stopped\n");
>                retval = -ENOLINK;
>        }
> -       spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
> +       if (!test_cpu_bit(URB_UNLINKING, dev->cpuflags))
> +               spin_unlock_irqrestore (&dev->rxq.lock, lockflags);
>        if (retval) {
>                dev_kfree_skb_any (skb);
>                usb_free_urb (urb);
> @@ -582,6 +586,7 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
>        int                     count = 0;
>
>        spin_lock_irqsave (&q->lock, flags);
> +       set_cpu_bit(URB_UNLINKING, dev->cpuflags);
>        skb_queue_walk_safe(q, skb, skbnext) {
>                struct skb_data         *entry;
>                struct urb              *urb;
> @@ -590,15 +595,6 @@ static int unlink_urbs (struct usbnet *dev,
> struct sk_buff_head *q)
>                entry = (struct skb_data *) skb->cb;
>                urb = entry->urb;
>
> -               /*
> -                * Get reference count of the URB to avoid it to be
> -                * freed during usb_unlink_urb, which may trigger
> -                * use-after-free problem inside usb_unlink_urb since
> -                * usb_unlink_urb is always racing with .complete
> -                * handler(include defer_bh).
> -                */
> -               usb_get_urb(urb);
> -               spin_unlock_irqrestore(&q->lock, flags);
>                // during some PM-driven resume scenarios,
>                // these (async) unlinks complete immediately
>                retval = usb_unlink_urb (urb);
> @@ -606,9 +602,8 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
>                        netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
>                else
>                        count++;
> -               usb_put_urb(urb);
> -               spin_lock_irqsave(&q->lock, flags);
>        }
> +       clear_cpu_bit(URB_UNLINKING, dev->cpuflags);
>        spin_unlock_irqrestore (&q->lock, flags);
>        return count;
>  }
> @@ -1283,6 +1278,7 @@ void usbnet_disconnect (struct usb_interface *intf)
>        usb_kill_urb(dev->interrupt);
>        usb_free_urb(dev->interrupt);
>
> +       free_percpu(dev->cpuflags);
>        free_netdev(net);
>        usb_put_dev (xdev);
>  }
> @@ -1353,6 +1349,13 @@ usbnet_probe (struct usb_interface *udev, const
> struct usb_device_id *prod)
>        SET_NETDEV_DEV(net, &udev->dev);
>
>        dev = netdev_priv(net);
> +
> +       dev->cpuflags = alloc_percpu(unsigned long);
> +       if (!dev->cpuflags) {
> +               status = -ENOMEM;
> +               goto out1;
> +       }
> +
>        dev->udev = xdev;
>        dev->intf = udev;
>        dev->driver_info = info;
> @@ -1396,7 +1399,7 @@ usbnet_probe (struct usb_interface *udev, const
> struct usb_device_id *prod)
>        if (info->bind) {
>                status = info->bind (dev, udev);
>                if (status < 0)
> -                       goto out1;
> +                       goto out2;
>
>                // heuristic:  "usb%d" for links we know are two-host,
>                // else "eth%d" when there's reasonable doubt.  userspace
> @@ -1465,6 +1468,8 @@ usbnet_probe (struct usb_interface *udev, const
> struct usb_device_id *prod)
>  out3:
>        if (info->unbind)
>                info->unbind (dev, udev);
> +out2:
> +       free_percpu(dev->cpuflags);
>  out1:
>        free_netdev(net);
>  out:
> diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
> index 605b0aa..2dc46f5 100644
> --- a/include/linux/usb/usbnet.h
> +++ b/include/linux/usb/usbnet.h
> @@ -69,8 +69,28 @@ struct usbnet {
>  #              define EVENT_DEV_WAKING 6
>  #              define EVENT_DEV_ASLEEP 7
>  #              define EVENT_DEV_OPEN   8
> +       unsigned long __percpu *cpuflags;
> +#              define URB_UNLINKING    0
>  };
>
> +static inline void set_cpu_bit(int nr, unsigned long __percpu *addr)
> +{
> +       unsigned long *fl = __this_cpu_ptr(addr);
> +       set_bit(nr, fl);
> +}
> +
> +static inline void clear_cpu_bit(int nr, unsigned long __percpu *addr)
> +{
> +       unsigned long *fl = __this_cpu_ptr(addr);
> +       clear_bit(nr, fl);
> +}
> +
> +static inline int test_cpu_bit(int nr, unsigned long __percpu *addr)
> +{
> +       unsigned long *fl = __this_cpu_ptr(addr);
> +       return test_bit(nr, fl);
> +}
> +
>  static inline struct usb_driver *driver_of(struct usb_interface *intf)
>  {
>        return to_usb_driver(intf->dev.driver);
>
>
> Thanks,
> --
> Ming Lei

Hi Ming,
   Many changes.
   Anyway, I will try it when I back to home.  :)
--
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: use-after-free in usbnet
From: Ming Lei @ 2012-04-21  7:56 UTC (permalink / raw)
  To: Huajun Li
  Cc: Oliver Neukum, Alan Stern, Dave Jones,
	netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
	Fedora Kernel Team
In-Reply-To: <CA+v9cxa+fyzMOD-=oLLczpu1FDtTwcok+y2FkC=mHzDH3JYJ2A-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

Hi Huajun,

On Sat, Apr 21, 2012 at 3:50 PM, Huajun Li <huajun.li.lee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Did we on the same page, could you please review my patch again?
>
> My draft patch was based on current mainline( 3.4.0-rc3)  which had
> already integrated your previous patch. And in my patch, it replaced
> skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
> = skb->next'  any more.

Replace skb_queue_walk_safe with skb_queue_walk doesn't improve
the problem, since 'skb = skb->next' in skb_queue_walk still may trigger
the oops, does it?


Thanks,
-- 
Ming Lei
--
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: use-after-free in usbnet
From: Huajun Li @ 2012-04-21  7:50 UTC (permalink / raw)
  To: Ming Lei
  Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
	Fedora Kernel Team
In-Reply-To: <CACVXFVN6kvXk7s4Sc0d_-yKSM=rV3qcuPPBHVZYzoQjnwkGX+Q@mail.gmail.com>

On Sat, Apr 21, 2012 at 3:06 PM, Ming Lei <tom.leiming@gmail.com> wrote:
> Hi Huajun,
>
> On Sat, Apr 21, 2012 at 2:39 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>
>> Hi Ming,
>>     That's why my patch uses skb_queue_walk() to traverse the queue,
>> it guarantees the skb available in each loop.  Is this what you
>> expected?
>>
>>     The main idea of my patch(it is based on current mainline: 3.4.0-rc3) is:
>>     1. If the skb in txq/rxq, then it must be available,
>> unlink_urbs() can refer to it safely while it holds  q->lock;
>>     2. If the skb in txq/rxq and its state is
>> rx_done/tx_done/rx_cleanup, that means the skb's URB is complete, then
>> don't need to unlink it again;
>
> As I said before, at least current code is not mistaken, since unlink
> can handle the case correctly.
>
>>     3. Before releasing  q->lock in unlink_urbs(), it will increase
>> the URB's refercount, so even the related skb is freed in future, the
>> URB is still available.
>
> No, increasing URB's reference count does not prevent the referenced skb
> from being freed, see usbnet_bh(), so 'tmp = skb->next' in skb_queue_walk_safe
> still may reference a freed pointer.
>

Did we on the same page, could you please review my patch again?

My draft patch was based on current mainline( 3.4.0-rc3)  which had
already integrated your previous patch. And in my patch, it replaced
skb_queue_walk_safe() with skb_queue_walk(), so you will not see  'tmp
= skb->next'  any more.

>>> As far as I can think of, we can hold lock of done queue to forbid skb free
>>> during unlinking. The below patch may fix the problem, are you OK
>>> with it?
>>
>> Just skip trying this per your following email's comments.
>
> I will prepare a new patch later, if you'd like to try it.
>
>
> Thanks
> --
> Ming Lei

^ permalink raw reply


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