Netdev List
 help / color / mirror / Atom feed
* Re: [PATCH] RPC: killing RPC tasks races fixed
From: Stanislav Kinsbursky @ 2011-03-17 15:43 UTC (permalink / raw)
  To: Trond Myklebust
  Cc: linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	bfields@fieldses.org, davem@davemloft.net, skinsbursky@openvz.org
In-Reply-To: <1300366904.9654.13.camel@lade.trondhjem.org>

17.03.2011 16:01, Trond Myklebust пишет:
> On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:
>> task->tk_waitqueue must be checked for NULL before trying to wake up task in
>> rpc_killall_tasks() because it can be NULL.
>>
>> Here is an example:
>>
>> CPU 0               	CPU 1				CPU 2
>> --------------------	---------------------	--------------------------
>> nfs4_run_open_task
>> rpc_run_task
>> rpc_execute
>> rpc_set_active
>> rpc_make_runnable
>> (waiting)
>> 			rpc_async_schedule
>> 			nfs4_open_prepare
>> 			nfs_wait_on_sequence
>> 						nfs_umount_begin
>> 						rpc_killall_tasks
>> 						rpc_wake_up_task
>> 						rpc_wake_up_queued_task
>> 						spin_lock(tk_waitqueue == NULL)
>> 						BUG()
>> 			rpc_sleep_on
>> 			spin_lock(&q->lock)
>> 			__rpc_sleep_on
>> 			task->tk_waitqueue = q
>>
>> Signed-off-by: Stanislav Kinsbursky<skinsbursky@openvz.org>
>>
>> ---
>>   net/sunrpc/clnt.c |    4 +++-
>>   1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index 57d344c..24039fe 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
>>   		if (!(rovr->tk_flags&  RPC_TASK_KILLED)) {
>>   			rovr->tk_flags |= RPC_TASK_KILLED;
>>   			rpc_exit(rovr, -EIO);
>> -			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
>> +			if (rovr->tk_waitqueue)
>> +				rpc_wake_up_queued_task(rovr->tk_waitqueue,
>> +							rovr);
>
> Testing for RPC_IS_QUEUED(rovr) would be better, since that would
> optimise away the call to rpc_wake_up_queued_task() altogether for those
> tasks that aren't queued.
>

Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks
clearer and in 2.6.38 tk_waitqueue is initialized prior to set
RPC_TASK_QUEUED bit.
But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed.
Will send fixed version soon.

>>   		}
>>   	}
>>   	spin_unlock(&clnt->cl_lock);
>>
>


-- 
Best regards,
Stanislav Kinsbursky

^ permalink raw reply

* [PATCH v2] RPC: killing RPC tasks races fixed
From: Stanislav Kinsbursky @ 2011-03-17 15:54 UTC (permalink / raw)
  To: Trond.Myklebust
  Cc: linux-nfs, xemul, neilb, netdev, linux-kernel, bfields, davem,
	skinsbursky

RPC task RPC_TASK_QUEUED bit is set must be checked before trying to wake up
task rpc_killall_tasks() because task->tk_waitqueue can not be set (equal to
NULL).
Also, as Trond Myklebust mentioned, such approach (instead of checking
tk_waitqueue to NULL) allows us to "optimise away the call to
rpc_wake_up_queued_task() altogether for those
tasks that aren't queued".

Here is an example of dereferencing of tk_waitqueue equal to NULL:

CPU 0               	CPU 1				CPU 2
--------------------	---------------------	--------------------------
nfs4_run_open_task
rpc_run_task
rpc_execute
rpc_set_active
rpc_make_runnable
(waiting)
			rpc_async_schedule
			nfs4_open_prepare
			nfs_wait_on_sequence
						nfs_umount_begin
						rpc_killall_tasks
						rpc_wake_up_task
						rpc_wake_up_queued_task
						spin_lock(tk_waitqueue == NULL)
						BUG()
			rpc_sleep_on
			spin_lock(&q->lock)
			__rpc_sleep_on
			task->tk_waitqueue = q

Signed-off-by: Stanislav Kinsbursky <skinsbursky@openvz.org>

---
 net/sunrpc/clnt.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 57d344c..3444757 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
 		if (!(rovr->tk_flags & RPC_TASK_KILLED)) {
 			rovr->tk_flags |= RPC_TASK_KILLED;
 			rpc_exit(rovr, -EIO);
-			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
+			if (RPC_IS_QUEUED(rovr))
+				rpc_wake_up_queued_task(rovr->tk_waitqueue, 
+							rovr);
 		}
 	}
 	spin_unlock(&clnt->cl_lock);

^ permalink raw reply related

* Re: Stale entries in RT_TABLE_LOCAL
From: Jiri Bohac @ 2011-03-17 16:11 UTC (permalink / raw)
  To: Julian Anastasov; +Cc: Alex Sidorenko, David Miller, netdev@vger.kernel.org
In-Reply-To: <alpine.LFD.2.00.1103151033260.1729@ja.ssi.bg>

Hi,

On Tue, Mar 15, 2011 at 10:47:20AM +0200, Julian Anastasov wrote:
> On Thu, 10 Mar 2011, Alex Sidorenko wrote:
> 
> >>IP1: 192.168.140.31/22, primary
> >>IP2: 192.168.142.109/23, primary
> >>IP3: 192.168.142.109/22, secondary for primary IP1
> >>
> >>        It is the route for IP3 that is leaked, with prefsrc=IP1.
> >>We create local route for secondary IPs with prefsrc=ItsPrimaryIP.
> >>Both local routes for 109 differ in prefsrc (fa_info)

Is there any reason to set the prefsrc of a local route to the
primary IP address of the subnet?

I tried the following patch:

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 1d2cdd4..2046b21 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -751,7 +751,7 @@ void fib_add_ifaddr(struct in_ifaddr *ifa)
 		}
 	}
 
-	fib_magic(RTM_NEWROUTE, RTN_LOCAL, addr, 32, prim);
+	fib_magic(RTM_NEWROUTE, RTN_LOCAL, addr, 32, ifa);
 
 	if (!(dev->flags & IFF_UP))
 		return;


The result with the teststcase mentioned previously is that only
one local route is created per IP address. The local routes are
correctly deleted after both identical IP addresses are removed
from the interface.

Furthemore, the testcase uncovers another weirdness with the
prefsrc of the local routes. When a primary IP address is deleted
and a secondary IP address is promoted to primary, its prefsrc is
not updated.

What is the prefrc of a local route good for?

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ


^ permalink raw reply related

* [PATCH] ethtool: __ethtool_set_sg: check for function pointer before using it
From: Roger Luethi @ 2011-03-17 16:37 UTC (permalink / raw)
  To: netdev

__ethtool_set_sg does not check if dev->ethtool_ops->set_sg is defined
which can result in a NULL pointer dereference when ethtool is used to
change SG settings for drivers without SG support.

Signed-off-by: Roger Luethi <rl@hellgate.ch>
---

Bug verified. Patch only compile-tested.

 net/core/ethtool.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index c1a71bb..a1086fb 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1457,6 +1457,9 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
 {
 	int err;
 
+	if (!dev->ethtool_ops->set_sg)
+		return -EOPNOTSUPP;
+
 	if (data && !(dev->features & NETIF_F_ALL_CSUM))
 		return -EINVAL;
 
-- 
1.7.3.4


^ permalink raw reply related

* Re: [PATCH] RPC: killing RPC tasks races fixed
From: Trond Myklebust @ 2011-03-17 16:44 UTC (permalink / raw)
  To: Stanislav Kinsbursky
  Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Pavel Emelianov, neilb-l3A5Bk7waGM@public.gmane.org,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org,
	davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org,
	skinsbursky-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org
In-Reply-To: <4D822C3C.9090100-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>

On Thu, 2011-03-17 at 18:43 +0300, Stanislav Kinsbursky wrote:
> 17.03.2011 16:01, Trond Myklebust пишет:
> > On Thu, 2011-03-17 at 15:16 +0300, Stanislav Kinsbursky wrote:
> >> task->tk_waitqueue must be checked for NULL before trying to wake up task in
> >> rpc_killall_tasks() because it can be NULL.
> >>
> >> Here is an example:
> >>
> >> CPU 0               	CPU 1				CPU 2
> >> --------------------	---------------------	--------------------------
> >> nfs4_run_open_task
> >> rpc_run_task
> >> rpc_execute
> >> rpc_set_active
> >> rpc_make_runnable
> >> (waiting)
> >> 			rpc_async_schedule
> >> 			nfs4_open_prepare
> >> 			nfs_wait_on_sequence
> >> 						nfs_umount_begin
> >> 						rpc_killall_tasks
> >> 						rpc_wake_up_task
> >> 						rpc_wake_up_queued_task
> >> 						spin_lock(tk_waitqueue == NULL)
> >> 						BUG()
> >> 			rpc_sleep_on
> >> 			spin_lock(&q->lock)
> >> 			__rpc_sleep_on
> >> 			task->tk_waitqueue = q
> >>
> >> Signed-off-by: Stanislav Kinsbursky<skinsbursky-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> >>
> >> ---
> >>   net/sunrpc/clnt.c |    4 +++-
> >>   1 files changed, 3 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> >> index 57d344c..24039fe 100644
> >> --- a/net/sunrpc/clnt.c
> >> +++ b/net/sunrpc/clnt.c
> >> @@ -436,7 +436,9 @@ void rpc_killall_tasks(struct rpc_clnt *clnt)
> >>   		if (!(rovr->tk_flags&  RPC_TASK_KILLED)) {
> >>   			rovr->tk_flags |= RPC_TASK_KILLED;
> >>   			rpc_exit(rovr, -EIO);
> >> -			rpc_wake_up_queued_task(rovr->tk_waitqueue, rovr);
> >> +			if (rovr->tk_waitqueue)
> >> +				rpc_wake_up_queued_task(rovr->tk_waitqueue,
> >> +							rovr);
> >
> > Testing for RPC_IS_QUEUED(rovr) would be better, since that would
> > optimise away the call to rpc_wake_up_queued_task() altogether for those
> > tasks that aren't queued.
> >
> 
> Yes, I agree with testing RPC_IS_QUEUED(rovr) since such approach looks
> clearer and in 2.6.38 tk_waitqueue is initialized prior to set
> RPC_TASK_QUEUED bit.
> But I found this problem in 2.6.32 rhel kernel where this set sequence is inversed.
> Will send fixed version soon.

Are you sure? Why would the 2.6.32 rhel kernel differ from the mainline
2.6.32 kernel in this respect?

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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

* Fw: [Bug 31282] New: Kernel panic . tcp_input.c:3317
From: Stephen Hemminger @ 2011-03-17 16:46 UTC (permalink / raw)
  To: netdev



Begin forwarded message:

Date: Thu, 17 Mar 2011 10:19:56 GMT
From: bugzilla-daemon@bugzilla.kernel.org
To: shemminger@linux-foundation.org
Subject: [Bug 31282] New: Kernel panic . tcp_input.c:3317


https://bugzilla.kernel.org/show_bug.cgi?id=31282

           Summary: Kernel panic . tcp_input.c:3317
           Product: Networking
           Version: 2.5
    Kernel Version: Linux version 2.6.32-28-generic (buildd@palmer) (gcc
                    version 4.4.3 (Ubuntu 4.4.3-4ubuntu5) ) #55-Ubuntu SMP
                    Mon Jan 10 21:21:01 UTC 2011 (Ubuntu
                    2.6.32-28.55-generic 2.6.32.27+drm33.12)
          Platform: All
        OS/Version: Linux
              Tree: Mainline
            Status: NEW
          Severity: normal
          Priority: P1
         Component: IPV4
        AssignedTo: shemminger@linux-foundation.org
        ReportedBy: vitaliy.migov@boldendeavours.com
        Regression: No


Mar  1 12:07:24 Securus kernel: [1036627.396040] ------------[ cut here
]------------
Mar  1 12:07:24 Securus kernel: [1036627.396049] WARNING: at
/build/buildd/linux-2.6.32/net/ipv4/tcp_input.c:3317
tcp_clean_rtx_queue+0x6b8/0x7b0()
Mar  1 12:07:24 Securus kernel: [1036627.396051] Hardware name: OEM
Mar  1 12:07:24 Securus kernel: [1036627.396052] Modules linked in: binfmt_misc
ipt_REDIRECT iptable_nat nf_nat nf_conntrack_ipv4 nf_conntrack nf_defrag_ipv4
xt_tcpudp iptable_filter ip_tables x_tables fbcon tileblit font bitblit
softcursor vga16fb vgastate i915 ppdev drm_kms_helper parport_pc serio_raw drm
i2c_algo_bit video lp output intel_agp agpgart parport e1000e
Mar  1 12:07:24 Securus kernel: [1036627.396075] Pid: 1589, comm: mysqld Not
tainted 2.6.32-28-generic #55-Ubuntu
Mar  1 12:07:24 Securus kernel: [1036627.396077] Call Trace:
Mar  1 12:07:24 Securus kernel: [1036627.396082]  [<c014c892>]
warn_slowpath_common+0x72/0xa0
Mar  1 12:07:24 Securus kernel: [1036627.396085]  [<c0503038>] ?
tcp_clean_rtx_queue+0x6b8/0x7b0
Mar  1 12:07:24 Securus kernel: [1036627.396087]  [<c0503038>] ?
tcp_clean_rtx_queue+0x6b8/0x7b0
Mar  1 12:07:24 Securus kernel: [1036627.396091]  [<c052bc50>] ?
bictcp_acked+0x0/0x180
Mar  1 12:07:24 Securus kernel: [1036627.396093]  [<c014c8da>]
warn_slowpath_null+0x1a/0x20
Mar  1 12:07:24 Securus kernel: [1036627.396096]  [<c0503038>]
tcp_clean_rtx_queue+0x6b8/0x7b0
Mar  1 12:07:24 Securus kernel: [1036627.396099]  [<c013fb83>] ?
finish_task_switch+0x43/0xc0
Mar  1 12:07:24 Securus kernel: [1036627.396102]  [<c050332d>]
tcp_ack+0x1fd/0x640
Mar  1 12:07:24 Securus kernel: [1036627.396105]  [<c0504177>]
tcp_rcv_established+0x4e7/0x670
Mar  1 12:07:24 Securus kernel: [1036627.396108]  [<c04b843c>] ?
release_sock+0x9c/0xb0
Mar  1 12:07:24 Securus kernel: [1036627.396111]  [<c050b0ed>]
tcp_v4_do_rcv+0xbd/0x130
Mar  1 12:07:24 Securus kernel: [1036627.396113]  [<c04f8942>]
tcp_prequeue_process+0x62/0x80
Mar  1 12:07:24 Securus kernel: [1036627.396116]  [<c04fb24c>]
tcp_recvmsg+0x8fc/0xbe0
Mar  1 12:07:24 Securus kernel: [1036627.396119]  [<c058e8e6>] ?
_spin_unlock_bh+0x16/0x20
Mar  1 12:07:24 Securus kernel: [1036627.396121]  [<c04b843c>] ?
release_sock+0x9c/0xb0
Mar  1 12:07:24 Securus kernel: [1036627.396124]  [<c04f88aa>] ?
tcp_push+0x7a/0xb0
Mar  1 12:07:24 Securus kernel: [1036627.396127]  [<c04b7c28>]
sock_common_recvmsg+0x48/0x60
Mar  1 12:07:24 Securus kernel: [1036627.396130]  [<c04b7884>]
sock_aio_read+0x124/0x130
Mar  1 12:07:24 Securus kernel: [1036627.396134]  [<c0209164>]
do_sync_read+0xc4/0x100
Mar  1 12:07:24 Securus kernel: [1036627.396137]  [<c0167be0>] ?
autoremove_wake_function+0x0/0x50
Mar  1 12:07:24 Securus kernel: [1036627.396140]  [<c0144c79>] ?
run_rebalance_domains+0x39/0xc0
Mar  1 12:07:24 Securus kernel: [1036627.396144]  [<c02f6544>] ?
security_file_permission+0x14/0x20
Mar  1 12:07:24 Securus kernel: [1036627.396147]  [<c0209204>] ?
rw_verify_area+0x64/0xe0
Mar  1 12:07:24 Securus kernel: [1036627.396149]  [<c0209be2>]
vfs_read+0x182/0x1a0
Mar  1 12:07:24 Securus kernel: [1036627.396152]  [<c0216c7d>] ?
do_fcntl+0xad/0x360
Mar  1 12:07:24 Securus kernel: [1036627.396154]  [<c0355309>] ?
copy_to_user+0x39/0x130
Mar  1 12:07:24 Securus kernel: [1036627.396157]  [<c0209cb2>]
sys_read+0x42/0x70
Mar  1 12:07:24 Securus kernel: [1036627.396160]  [<c01033ec>]
syscall_call+0x7/0xb
Mar  1 12:07:24 Securus kernel: [1036627.396162] ---[ end trace
a54f02b2f089b137 ]---

-- 
Configure bugmail: https://bugzilla.kernel.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.


-- 

^ permalink raw reply

* Re: [PATCH] virtio: Avoid virtio_net TX queue over run
From: Shirley Ma @ 2011-03-17 16:50 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: Rusty Russell, David Miller, kvm, netdev
In-Reply-To: <20110317050538.GD32049@redhat.com>

On Thu, 2011-03-17 at 07:05 +0200, Michael S. Tsirkin wrote:
> Could you add CPU utilization data pls? I also wonder what does this
> do
> to UDP?  Won't a lot of packets be dropped?

Guest CPU utilization slightly increased a few %.

UDP performance send rate increased 200% - 400%, recv rate increased
from 10% to around 40% w/i recv errors. I will tune the buffers to see
any better.

I am putting all data together and post the results here soon.

Thanks
Shirley


^ permalink raw reply

* Re: [PATCH] ethtool: __ethtool_set_sg: check for function pointer before using it
From: Ben Hutchings @ 2011-03-17 16:52 UTC (permalink / raw)
  To: Roger Luethi, Michał Mirosław; +Cc: netdev
In-Reply-To: <20110317163720.GA26284@core.hellgate.ch>

On Thu, 2011-03-17 at 17:37 +0100, Roger Luethi wrote:
> __ethtool_set_sg does not check if dev->ethtool_ops->set_sg is defined
> which can result in a NULL pointer dereference when ethtool is used to
> change SG settings for drivers without SG support.
> 
> Signed-off-by: Roger Luethi <rl@hellgate.ch>
Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>

Michał, was this just an oversight or is there some reason why we
shouldn't check set_sg immediately?

Ben.

> ---
> 
> Bug verified. Patch only compile-tested.
>
>  net/core/ethtool.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..a1086fb 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1457,6 +1457,9 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
>  {
>  	int err;
>  
> +	if (!dev->ethtool_ops->set_sg)
> +		return -EOPNOTSUPP;
> +
>  	if (data && !(dev->features & NETIF_F_ALL_CSUM))
>  		return -EINVAL;
>  

-- 
Ben Hutchings, Senior Software 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: Poll about irqsafe_cpu_add and others
From: Eric Dumazet @ 2011-03-17 16:53 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: David Miller, linux-kernel, linux-arch, netdev, netfilter-devel
In-Reply-To: <alpine.DEB.2.00.1103171016270.12540@router.home>

Le jeudi 17 mars 2011 à 10:18 -0500, Christoph Lameter a écrit :
> On Thu, 17 Mar 2011, David Miller wrote:
> 
> >
> > I had been meaning to bring this up from another perspective.
> >
> > In networking, we often only ever access objects in base or
> > BH context.  Therefore in BH context cases we can do just
> > normal counter bumps without any of the special atomic or
> > IRQ disabling code at all.
> 
> We have the __ functions for that purpose. __this_cpu_inc f.e. falls back
> to a simply ++ operation if the arch cannot provide something better.
> irqsafe_xx are only used if the context does not provide any protection
> and if there is the potential of the counter being incremented from an
> interrupt context.
> 

What David and I have in mind is to use one array per mib instead of
two. This is an old idea.

https://patchwork.kernel.org/patch/15883/

When we know we run from BH context, we can use __this_cpu_inc(), but if
we dont know or run from user/process context, we would need irqsafe_inc
variant.

For x86 this maps to same single instruction, but for other arches, this
might be too expensive.

BTW, I think following patch is possible to save some text and useless
tests (on 64bit platform at least)

size vmlinux.old vmlinux

Thanks

[PATCH] snmp: SNMP_UPD_PO_STATS_BH() always called from softirq

We dont need to test if we run from softirq context.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
---
 include/net/snmp.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/net/snmp.h b/include/net/snmp.h
index 762e2ab..be2424d 100644
--- a/include/net/snmp.h
+++ b/include/net/snmp.h
@@ -149,8 +149,8 @@ struct linux_xfrm_mib {
 	} while (0)
 #define SNMP_UPD_PO_STATS_BH(mib, basefield, addend)	\
 	do { \
-		__typeof__(*mib[0]) *ptr = \
-			__this_cpu_ptr((mib)[!in_softirq()]); \
+		__typeof__(*mib[0]) *ptr = __this_cpu_ptr((mib)[0]);	\
+									\
 		ptr->mibs[basefield##PKTS]++; \
 		ptr->mibs[basefield##OCTETS] += addend;\
 	} while (0)

^ permalink raw reply related

* Re: [PATCH] virtio: Avoid virtio_net TX queue over run
From: Eric Dumazet @ 2011-03-17 16:58 UTC (permalink / raw)
  To: Shirley Ma; +Cc: Michael S. Tsirkin, Rusty Russell, David Miller, kvm, netdev
In-Reply-To: <1300380626.3255.53.camel@localhost.localdomain>

Le jeudi 17 mars 2011 à 09:50 -0700, Shirley Ma a écrit :
> On Thu, 2011-03-17 at 07:05 +0200, Michael S. Tsirkin wrote:
> > Could you add CPU utilization data pls? I also wonder what does this
> > do
> > to UDP?  Won't a lot of packets be dropped?
> 
> Guest CPU utilization slightly increased a few %.
> 
> UDP performance send rate increased 200% - 400%, recv rate increased
> from 10% to around 40% w/i recv errors. I will tune the buffers to see
> any better.
> 
> I am putting all data together and post the results here soon.

Could you run your tests with only one cpu ?
(offline all cpus but cpu0 on your hypervisor)

Dropping packets is fine, unless consumer cant run ;)




^ permalink raw reply

* Re: [PATCH] ethtool: __ethtool_set_sg: check for function pointer before using it
From: Michał Mirosław @ 2011-03-17 17:00 UTC (permalink / raw)
  To: Roger Luethi; +Cc: netdev, David Miller
In-Reply-To: <20110317163720.GA26284@core.hellgate.ch>

2011/3/17 Roger Luethi <rl@hellgate.ch>:
> __ethtool_set_sg does not check if dev->ethtool_ops->set_sg is defined
> which can result in a NULL pointer dereference when ethtool is used to
> change SG settings for drivers without SG support.
>
> Signed-off-by: Roger Luethi <rl@hellgate.ch>
> ---
>
> Bug verified. Patch only compile-tested.
>
>  net/core/ethtool.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index c1a71bb..a1086fb 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1457,6 +1457,9 @@ static int __ethtool_set_sg(struct net_device *dev, u32 data)
>  {
>        int err;
>
> +       if (!dev->ethtool_ops->set_sg)
> +               return -EOPNOTSUPP;
> +
>        if (data && !(dev->features & NETIF_F_ALL_CSUM))
>                return -EINVAL;
>

Yes. __ethtool_set_sg() is the only function that was already there
before my unification series and I did tests only on drivers which had
set_sg() defined. :-/

This should go into 2.6.39 as a bugfix (adding Cc: DaveM).

Best Regards,
Michał Mirosław

^ permalink raw reply

* Re: [PATCH] virtio: Avoid virtio_net TX queue over run
From: Shirley Ma @ 2011-03-17 17:01 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Michael S. Tsirkin, Rusty Russell, David Miller, kvm, netdev
In-Reply-To: <1300381096.6315.315.camel@edumazet-laptop>

On Thu, 2011-03-17 at 17:58 +0100, Eric Dumazet wrote:
> Could you run your tests with only one cpu ?
> (offline all cpus but cpu0 on your hypervisor)
> 
> Dropping packets is fine, unless consumer cant run ;) 

Sure, will report the data soon.

Thanks
Shirley


^ permalink raw reply

* Re: [PATCH] RPC: killing RPC tasks races fixed
From: Stanislav Kinsbursky @ 2011-03-17 17:04 UTC (permalink / raw)
  To: Trond Myklebust
  Cc: linux-nfs@vger.kernel.org, Pavel Emelianov, neilb@suse.de,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	bfields@fieldses.org, davem@davemloft.net, skinsbursky@openvz.org
In-Reply-To: <1300380254.28305.1.camel@lade.trondhjem.org>

17.03.2011 19:44, Trond Myklebust пишет:
>
> Are you sure? Why would the 2.6.32 rhel kernel differ from the mainline
> 2.6.32 kernel in this respect?
>

Checked again and realized, that was wrong.
This set sequence is the same in 2.6.32 rhel, 2.6.32 and 2.6.38 kernels.

-- 
Best regards,
Stanislav Kinsbursky

^ permalink raw reply

* [ANNOUNCE] iproute 2.6.38
From: Stephen Hemminger @ 2011-03-17 17:16 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel

Due to popular demand, there is an iproute2 release for 2.6.38 after all. 

This version of iproute2 utilities intended for use with 2.6.38 or
later kernel, but should be backward compatible with older releases.
New features are support for macvlan passthru and xfrm security context.

Source: (note old developer.osdl.org address works as well)
  http://devresources.linuxfoundation.org/dev/iproute2/download/iproute2-2.6.38.tar.bz2

Repository:
  git://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git

For more info on iproute2 see:
  http://www.linuxfoundation.org/collaborate/workgroups/networking/iproute2

Report problems (or enhancements) to the netdev@vger.kernel.org mailing list.

Changelog:

Gerrit Renker (3):
      utils: get_jiffies always uses base=0
      iproute: fix unit conversion of rtt/rttvar/rto_min
      iproute: rename 'get_jiffies' since it uses msecs

Joy Latten (3):
      xfrm security context support
      xfrm security context support
      xfrm security context support

Nicolas Dichtel (1):
      iproute2: allow to specify truncation bits on auth algo

Sridhar Samudrala (1):
      macvlan/macvtap: support 'passthru' mode

Stephen Hemminger (2):
      Update to lasest kernel headers
      v2.6.38.1

Vlad Dogaru (1):
      iproute2: fix man page whitespace


^ permalink raw reply

* [PATCH net-2.6] ethtool: Compat handling for struct ethtool_rxnfc
From: Ben Hutchings @ 2011-03-17 17:34 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, Alexander Duyck, Santwona Behera

This structure was accidentally defined such that its layout can
differ between 32-bit and 64-bit processes.  Add compat structure
definitions and an ioctl wrapper function.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
Cc: stable@kernel.org [2.6.30+]
---
David,

I still haven't received any response on whether the ETHTOOL_GRXCLSRLALL
wrapping works with a real driver, but perhaps you could test it against
niu?  I think sparc32 and sparc64 have the same alignment for u64 so
this wrapper isn't strictly necessary, but it would still be used.  (Or
we can arrange to disable the conversion when compat_ethtool_rxnfc is
equivalent to ethtool_rxnfc.)

Ben.

 include/linux/ethtool.h |   34 ++++++++++++++
 net/socket.c            |  114 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 141 insertions(+), 7 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index aac3e2e..b297f28 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -13,6 +13,9 @@
 #ifndef _LINUX_ETHTOOL_H
 #define _LINUX_ETHTOOL_H
 
+#ifdef __KERNEL__
+#include <linux/compat.h>
+#endif
 #include <linux/types.h>
 #include <linux/if_ether.h>
 
@@ -450,6 +453,37 @@ struct ethtool_rxnfc {
 	__u32				rule_locs[0];
 };
 
+#ifdef __KERNEL__
+#ifdef CONFIG_COMPAT
+
+struct compat_ethtool_rx_flow_spec {
+	u32		flow_type;
+	union {
+		struct ethtool_tcpip4_spec		tcp_ip4_spec;
+		struct ethtool_tcpip4_spec		udp_ip4_spec;
+		struct ethtool_tcpip4_spec		sctp_ip4_spec;
+		struct ethtool_ah_espip4_spec		ah_ip4_spec;
+		struct ethtool_ah_espip4_spec		esp_ip4_spec;
+		struct ethtool_usrip4_spec		usr_ip4_spec;
+		struct ethhdr				ether_spec;
+		u8					hdata[72];
+	} h_u, m_u;
+	compat_u64	ring_cookie;
+	u32		location;
+};
+
+struct compat_ethtool_rxnfc {
+	u32				cmd;
+	u32				flow_type;
+	compat_u64			data;
+	struct compat_ethtool_rx_flow_spec fs;
+	u32				rule_cnt;
+	u32				rule_locs[0];
+};
+
+#endif /* CONFIG_COMPAT */
+#endif /* __KERNEL__ */
+
 /**
  * struct ethtool_rxfh_indir - command to get or set RX flow hash indirection
  * @cmd: Specific command number - %ETHTOOL_GRXFHINDIR or %ETHTOOL_SRXFHINDIR
diff --git a/net/socket.c b/net/socket.c
index 937d0fcf..5212447 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2588,23 +2588,123 @@ static int dev_ifconf(struct net *net, struct compat_ifconf __user *uifc32)
 
 static int ethtool_ioctl(struct net *net, struct compat_ifreq __user *ifr32)
 {
+	struct compat_ethtool_rxnfc __user *compat_rxnfc;
+	bool convert_in = false, convert_out = false;
+	size_t buf_size = ALIGN(sizeof(struct ifreq), 8);
+	struct ethtool_rxnfc __user *rxnfc;
 	struct ifreq __user *ifr;
+	u32 rule_cnt = 0, actual_rule_cnt;
+	u32 ethcmd;
 	u32 data;
-	void __user *datap;
+	int ret;
+
+	if (get_user(data, &ifr32->ifr_ifru.ifru_data))
+		return -EFAULT;
 
-	ifr = compat_alloc_user_space(sizeof(*ifr));
+	compat_rxnfc = compat_ptr(data);
 
-	if (copy_in_user(&ifr->ifr_name, &ifr32->ifr_name, IFNAMSIZ))
+	if (get_user(ethcmd, &compat_rxnfc->cmd))
 		return -EFAULT;
 
-	if (get_user(data, &ifr32->ifr_ifru.ifru_data))
+	/* Most ethtool structures are defined without padding.
+	 * Unfortunately struct ethtool_rxnfc is an exception.
+	 */
+	switch (ethcmd) {
+	default:
+		break;
+	case ETHTOOL_GRXCLSRLALL:
+		/* Buffer size is variable */
+		if (get_user(rule_cnt, &compat_rxnfc->rule_cnt))
+			return -EFAULT;
+		if (rule_cnt > KMALLOC_MAX_SIZE / sizeof(u32))
+			return -ENOMEM;
+		buf_size += rule_cnt * sizeof(u32);
+		/* fall through */
+	case ETHTOOL_GRXRINGS:
+	case ETHTOOL_GRXCLSRLCNT:
+	case ETHTOOL_GRXCLSRULE:
+		convert_out = true;
+		/* fall through */
+	case ETHTOOL_SRXCLSRLDEL:
+	case ETHTOOL_SRXCLSRLINS:
+		buf_size += sizeof(struct ethtool_rxnfc);
+		convert_in = true;
+		break;
+	}
+
+	ifr = compat_alloc_user_space(buf_size);
+	rxnfc = (void *)ifr + ALIGN(sizeof(struct ifreq), 8);
+
+	if (copy_in_user(&ifr->ifr_name, &ifr32->ifr_name, IFNAMSIZ))
 		return -EFAULT;
 
-	datap = compat_ptr(data);
-	if (put_user(datap, &ifr->ifr_ifru.ifru_data))
+	if (put_user(convert_in ? rxnfc : compat_ptr(data),
+		     &ifr->ifr_ifru.ifru_data))
 		return -EFAULT;
 
-	return dev_ioctl(net, SIOCETHTOOL, ifr);
+	if (convert_in) {
+		/* We expect there to be holes between fs.m_u and
+		 * fs.ring_cookie and at the end of fs, but nowhere else.
+		 */
+		BUILD_BUG_ON(offsetof(struct compat_ethtool_rxnfc, fs.m_u) +
+			     sizeof(compat_rxnfc->fs.m_u) !=
+			     offsetof(struct ethtool_rxnfc, fs.m_u) +
+			     sizeof(rxnfc->fs.m_u));
+		BUILD_BUG_ON(
+			offsetof(struct compat_ethtool_rxnfc, fs.location) -
+			offsetof(struct compat_ethtool_rxnfc, fs.ring_cookie) !=
+			offsetof(struct ethtool_rxnfc, fs.location) -
+			offsetof(struct ethtool_rxnfc, fs.ring_cookie));
+
+		if (copy_in_user(rxnfc, compat_rxnfc,
+				 (void *)(&rxnfc->fs.m_u + 1) -
+				 (void *)rxnfc) ||
+		    copy_in_user(&rxnfc->fs.ring_cookie,
+				 &compat_rxnfc->fs.ring_cookie,
+				 (void *)(&rxnfc->fs.location + 1) -
+				 (void *)&rxnfc->fs.ring_cookie) ||
+		    copy_in_user(&rxnfc->rule_cnt, &compat_rxnfc->rule_cnt,
+				 sizeof(rxnfc->rule_cnt)))
+			return -EFAULT;
+	}
+
+	ret = dev_ioctl(net, SIOCETHTOOL, ifr);
+	if (ret)
+		return ret;
+
+	if (convert_out) {
+		if (copy_in_user(compat_rxnfc, rxnfc,
+				 (const void *)(&rxnfc->fs.m_u + 1) -
+				 (const void *)rxnfc) ||
+		    copy_in_user(&compat_rxnfc->fs.ring_cookie,
+				 &rxnfc->fs.ring_cookie,
+				 (const void *)(&rxnfc->fs.location + 1) -
+				 (const void *)&rxnfc->fs.ring_cookie) ||
+		    copy_in_user(&compat_rxnfc->rule_cnt, &rxnfc->rule_cnt,
+				 sizeof(rxnfc->rule_cnt)))
+			return -EFAULT;
+
+		if (ethcmd == ETHTOOL_GRXCLSRLALL) {
+			/* As an optimisation, we only copy the actual
+			 * number of rules that the underlying
+			 * function returned.  Since Mallory might
+			 * change the rule count in user memory, we
+			 * check that it is less than the rule count
+			 * originally given (as the user buffer size),
+			 * which has been range-checked.
+			 */
+			if (get_user(actual_rule_cnt, &rxnfc->rule_cnt))
+				return -EFAULT;
+			if (actual_rule_cnt < rule_cnt)
+				rule_cnt = actual_rule_cnt;
+			if (copy_in_user(&compat_rxnfc->rule_locs[0],
+					 &rxnfc->rule_locs[0],
+					 rule_cnt * sizeof(u32)))
+				return -EFAULT;
+		}
+	}
+
+	return 0;
 }
 
 static int compat_siocwandev(struct net *net, struct compat_ifreq __user *uifr32)
-- 
1.7.4


-- 
Ben Hutchings, Senior Software 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 related

* Re: [PATCH net-dev2.6] bxn2x: Fix location of PCI-ids defines
From: Michael Chan @ 2011-03-17 17:32 UTC (permalink / raw)
  To: David Miller; +Cc: Eilon Greenstein, Ariel Elior, netdev@vger.kernel.org
In-Reply-To: <20110317.081046.193692423.davem@davemloft.net>


On Thu, 2011-03-17 at 08:10 -0700, David Miller wrote:
> From: "Eilon Greenstein" <eilong@broadcom.com>
> Date: Thu, 17 Mar 2011 15:13:33 +0200
> 
> > On Thu, 2011-03-17 at 06:06 -0700, David Miller wrote:
> >> From: "Ariel Elior" <ariele@broadcom.com>
> >> Date: Thu, 17 Mar 2011 14:27:04 +0200
> >> 
> >> > Signed-off-by: Ariel Elior <ariele@broadcom.com>
> >> > Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> >> 
> >> Unless you intend to reference these symbols elsewhere in the
> >> kernel, they should stay private in your driver.
> >> 
> > 
> > The bnx2i will need it as well. Isn't it better to keep all the PCI IDs
> > in the same global location?
> 
> If they are used in more than one driver, yes.

We already sent a similar patch through the scsi tree.  It is now in the
linux-next tree:

http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commit;h=8a4a0f3ad071e258a9637c5491c34005a9a97903




^ permalink raw reply

* Re: Poll about irqsafe_cpu_add and others
From: Christoph Lameter @ 2011-03-17 17:46 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, linux-kernel, linux-arch, netdev, netfilter-devel
In-Reply-To: <1300380801.6315.306.camel@edumazet-laptop>

On Thu, 17 Mar 2011, Eric Dumazet wrote:

> When we know we run from BH context, we can use __this_cpu_inc(), but if
> we dont know or run from user/process context, we would need irqsafe_inc
> variant.

If the BH context is the only one where we care about performance then its
ok I would think.

^ permalink raw reply

* Re: bnx2 vlan issue
From: Seblu @ 2011-03-17 18:02 UTC (permalink / raw)
  To: netdev
In-Reply-To: <AANLkTik0BOuDj5wmfHabZx_OzXDROtDLR2qtqUqQu4ko@mail.gmail.com>

On Thu, Mar 17, 2011 at 3:51 PM, Seblu <seblu@seblu.net> wrote:
> Good Afternoon Gentleman,
>
> I've an issue with a 2.6.38 (vanilla) on a debian unstable distro. I
> attached an lspci and lshw about hardware. I also attached debian
> network config file.
>
> My host is a host kvm which run vm on different networks (vlan). I
> have 2 1Gbit/s card (eth0, eth1) and 1 10Gbit/s card (eth2).
> I use bonding (bond0) mode 1 on the 1Gbit/s cards.
>
> vlan 14 and 15 are only availlable trought 1G cards. Same tagging on
> both cards in switch. 14 is untagged (need for pxe) and 15 is tagged.
>
> Every network on my host is in a bridge. Eg:
> vlan 15 is in br15 by bond0.15 as member,
> vlan 14 is in br14 by bond0 as member,
> vlan 20 is in br20 by eth2.20
>
> The issue is simple, packets from vlan 15 which are tagged are not
> visible on bond0.15 but in bond0 (see capture). Like if there is no
> vlan.
>
>
> in dmesg in see something like : "8021q: adding VLAN 0 to HW filter on
> device bond0".
> Maybe there is something wrong with hardware filtering?
>
> I don't find a way to disable hw vlan filtering.
> # ethtool -K eth1 rxvlan off
> Cannot set device flag settings: Invalid argument
>
> I found a way to make thing work and don't work by one command.
> # rmmod bonding
> # ifdown br14
> # ifdown br15
> # ifup --force bond0
> # ifup --force bond0.15
> # ifup --force br15
> => i can ping my ip on 15 working
> if i make ifup --force br14, i lost connectivity on vlan 15.
>
> I don't have isssue when bond0 is not member of br14. So vlan on
> bonding seems to be broken.
>
I done more test this afternoon.

I set an ip on bond0, ip on bond0.15 is still pingeable.
I created a bridge. Until I don't add bond0 in my bridge, bond0.15 is
still pingeable.



Now is see a relation with a similar issue come with 2.6.37. This become
eth0 => network 1
eth0.42 =>  network 2
br0 => eth0 + eth1 (network1)

Since 2.6.37, eth0.42 (network2) become unavailable, and it is only
possible to do something like:
br0 => eth0 + eth1 (network1)
br0.42 => network 2

To conclude:
add a bonding interface in a bridge, kill vlan setted on it.
add a real interface in a bridge, kill vlan setted on it.

So, i don't see how we can restore a per bridge vlan. Why do you
remove this possibility in 2.6.37?

-- 
Sébastien Luttringer
www.seblu.net

^ permalink raw reply

* Re: Poll about irqsafe_cpu_add and others
From: Eric Dumazet @ 2011-03-17 18:29 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: David Miller, linux-kernel, linux-arch, netdev, netfilter-devel
In-Reply-To: <alpine.DEB.2.00.1103171245430.16620@router.home>

Le jeudi 17 mars 2011 à 12:46 -0500, Christoph Lameter a écrit :
> On Thu, 17 Mar 2011, Eric Dumazet wrote:
> 
> > When we know we run from BH context, we can use __this_cpu_inc(), but if
> > we dont know or run from user/process context, we would need irqsafe_inc
> > variant.
> 
> If the BH context is the only one where we care about performance then its
> ok I would think.

Hmm... yes.

By the way, I noticed :

DECLARE_PER_CPU(u64, xt_u64);
__this_cpu_add(xt_u64, 2) translates to following x86_32 code :

	mov	$xt_u64,%eax
	add	%fs:0x0,%eax
	addl	$0x2,(%eax)
	adcl	$0x0,0x4(%eax)


I wonder why we dont use :

	addl	$0x2,%fs:xt_u64
	addcl	$0x0,%fs:xt_u64+4



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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: [PATCH] SUNRPC: Remove resource leak in svc_rdma_send_error()
From: J. Bruce Fields @ 2011-03-17 18:37 UTC (permalink / raw)
  To: Jesper Juhl
  Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Pavel Emelyanov, Chuck Lever,
	Tejun Heo, Tom Tucker, David S. Miller, Trond Myklebust,
	Neil Brown
In-Reply-To: <alpine.LNX.2.00.1101222233260.7746-h2p7t3/P30RzeRGmFJ5qR7ZzlVVXadcDXqFh9Ls21Oc@public.gmane.org>

On Sat, Jan 22, 2011 at 10:40:20PM +0100, Jesper Juhl wrote:
> We leak the memory allocated to 'ctxt' when we return after 
> 'ib_dma_mapping_error()' returns !=0.
> 
> Signed-off-by: Jesper Juhl <jj-IYz4IdjRLj0sV2N9l4h3zg@public.gmane.org>

I don't know this code, but I can't see how that could be wrong....
Applying unless Tom tells me otherwise.

--b.

> ---
>  svc_rdma_transport.c |    1 +
>  1 file changed, 1 insertion(+)
> 
>   compile tested only
> 
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 9df1ead..1a10dcd 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -1335,6 +1335,7 @@ void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
>  					    p, 0, length, DMA_FROM_DEVICE);
>  	if (ib_dma_mapping_error(xprt->sc_cm_id->device, ctxt->sge[0].addr)) {
>  		put_page(p);
> +		svc_rdma_put_context(ctxt, 1);
>  		return;
>  	}
>  	atomic_inc(&xprt->sc_dma_used);
> 
> 
> -- 
> Jesper Juhl <jj-IYz4IdjRLj0sV2N9l4h3zg@public.gmane.org>            http://www.chaosbits.net/
> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
> Plain text mails only, please.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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: Poll about irqsafe_cpu_add and others
From: Christoph Lameter @ 2011-03-17 18:42 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David Miller, linux-kernel, linux-arch, netdev, netfilter-devel
In-Reply-To: <1300386569.6315.404.camel@edumazet-laptop>

On Thu, 17 Mar 2011, Eric Dumazet wrote:

> By the way, I noticed :
>
> DECLARE_PER_CPU(u64, xt_u64);
> __this_cpu_add(xt_u64, 2) translates to following x86_32 code :
>
> 	mov	$xt_u64,%eax
> 	add	%fs:0x0,%eax
> 	addl	$0x2,(%eax)
> 	adcl	$0x0,0x4(%eax)
>
>
> I wonder why we dont use :
>
> 	addl	$0x2,%fs:xt_u64
> 	addcl	$0x0,%fs:xt_u64+4

The compiler is fed the following

	*__this_cpu_ptr(xt_u64) += 2

__this_cpu_ptr makes it:

	*(xt_u64 + __my_cpu_offset) += 2

So the compiler calculates the address first and then increments it.

The compiler could optimize this I think. Wonder why that does not happen.





^ permalink raw reply

* Re: [PATCH] virtio: Avoid virtio_net TX queue over run
From: Shirley Ma @ 2011-03-17 18:49 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Michael S. Tsirkin, Rusty Russell, David Miller, kvm, netdev
In-Reply-To: <1300381265.3255.54.camel@localhost.localdomain>

On Thu, 2011-03-17 at 10:01 -0700, Shirley Ma wrote:
> On Thu, 2011-03-17 at 17:58 +0100, Eric Dumazet wrote:
> > Could you run your tests with only one cpu ?
> > (offline all cpus but cpu0 on your hypervisor)
> > 
> > Dropping packets is fine, unless consumer cant run ;) 
> 
> Sure, will report the data soon. 

With a single CPU, before/after with this patch the performance has no
difference for both UDP and TCP, and it seems hitting TX overrun, CPU is
100% used.

Thanks
Shirley


^ permalink raw reply

* Re: [PATCH] virtio: Avoid virtio_net TX queue over run
From: Shirley Ma @ 2011-03-17 18:52 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Michael S. Tsirkin, Rusty Russell, David Miller, kvm, netdev
In-Reply-To: <1300387758.3255.56.camel@localhost.localdomain>

On Thu, 2011-03-17 at 11:49 -0700, Shirley Ma wrote:
> With a single CPU, before/after with this patch the performance has no
> difference for both UDP and TCP, and it seems hitting TX overrun, CPU
> is
> 100% used. 

Sorry, should be: it seems never hitting TX overrun.

Shirley


^ permalink raw reply

* Re: [PATCH] SUNRPC: Remove resource leak in svc_rdma_send_error()
From: Tom Tucker @ 2011-03-17 18:54 UTC (permalink / raw)
  To: J. Bruce Fields
  Cc: Jesper Juhl, linux-nfs, netdev, linux-kernel, Pavel Emelyanov,
	Chuck Lever, Tejun Heo, David S. Miller, Trond Myklebust,
	Neil Brown
In-Reply-To: <20110317183752.GH30180@fieldses.org>

Hi Bruce,

I believe this fix is correct.

Tom

On 3/17/11 12:37 PM, J. Bruce Fields wrote:
> On Sat, Jan 22, 2011 at 10:40:20PM +0100, Jesper Juhl wrote:
>> We leak the memory allocated to 'ctxt' when we return after
>> 'ib_dma_mapping_error()' returns !=0.
>>
>> Signed-off-by: Jesper Juhl<jj@chaosbits.net>
> I don't know this code, but I can't see how that could be wrong....
> Applying unless Tom tells me otherwise.
>
> --b.
>
>> ---
>>   svc_rdma_transport.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>>    compile tested only
>>
>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> index 9df1ead..1a10dcd 100644
>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>> @@ -1335,6 +1335,7 @@ void svc_rdma_send_error(struct svcxprt_rdma *xprt, struct rpcrdma_msg *rmsgp,
>>   					    p, 0, length, DMA_FROM_DEVICE);
>>   	if (ib_dma_mapping_error(xprt->sc_cm_id->device, ctxt->sge[0].addr)) {
>>   		put_page(p);
>> +		svc_rdma_put_context(ctxt, 1);
>>   		return;
>>   	}
>>   	atomic_inc(&xprt->sc_dma_used);
>>
>>
>> -- 
>> Jesper Juhl<jj@chaosbits.net>             http://www.chaosbits.net/
>> Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
>> Plain text mails only, please.
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply

* Re: Poll about irqsafe_cpu_add and others
From: Eric Dumazet @ 2011-03-17 18:55 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: David Miller, linux-kernel, linux-arch, netdev, netfilter-devel
In-Reply-To: <alpine.DEB.2.00.1103171338350.18529@router.home>

Le jeudi 17 mars 2011 à 13:42 -0500, Christoph Lameter a écrit :
> On Thu, 17 Mar 2011, Eric Dumazet wrote:
> 
> > By the way, I noticed :
> >
> > DECLARE_PER_CPU(u64, xt_u64);
> > __this_cpu_add(xt_u64, 2) translates to following x86_32 code :
> >
> > 	mov	$xt_u64,%eax
> > 	add	%fs:0x0,%eax
> > 	addl	$0x2,(%eax)
> > 	adcl	$0x0,0x4(%eax)
> >
> >
> > I wonder why we dont use :
> >
> > 	addl	$0x2,%fs:xt_u64
> > 	addcl	$0x0,%fs:xt_u64+4
> 
> The compiler is fed the following
> 
> 	*__this_cpu_ptr(xt_u64) += 2
> 
> __this_cpu_ptr makes it:
> 
> 	*(xt_u64 + __my_cpu_offset) += 2
> 
> So the compiler calculates the address first and then increments it.
> 
> The compiler could optimize this I think. Wonder why that does not happen.

Compiler is really forced to compute addr, thats why.

Hmm, we should not fallback to generic ops I think, but tweak 

percpu_add_op() { 
...
	case 8: 
#if CONFIG_X86_64_SMP
                if (pao_ID__ == 1)                                      \
                        asm("incq "__percpu_arg(0) : "+m" (var));       \
                else if (pao_ID__ == -1)                                \
                        asm("decq "__percpu_arg(0) : "+m" (var));       \
                else                                                    \
                        asm("addq %1, "__percpu_arg(0)                  \
                            : "+m" (var)                                \
                            : "re" ((pao_T__)(val)));                   \
                break;                                                  \

#else
                asm("addl %1, "__percpu_arg(0)                  	\
                            : "+m" (var)                                \
                            : "ri" ((u32)(val)));                   	\
		asm("adcl %1, "__percpu_arg(0) 				\
			: "+m" ((char *)var+4)				\
			: "ri" ((u32)(val>>32));			\
                break;                                                  \

#endif
....
}

^ 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