* Re: [PATCH] net/ceph/osdmap.c: fix undefined behavior when using snprintf()
From: Alex Elder @ 2013-01-23 16:48 UTC (permalink / raw)
To: Cong Ding; +Cc: Sage Weil, David S. Miller, ceph-devel, netdev, linux-kernel
In-Reply-To: <1358882429-19066-1-git-send-email-dinggnu@gmail.com>
On 01/22/2013 01:20 PM, Cong Ding wrote:
> The variable "str" is used as both the source and destination in function
> snprintf(), which is undefined behavior based on C11. The original description
> in C11 is:
> "If copying takes place between objects that
> overlap, the behavior is undefined."
Yes, this was an ill-advised thing to do in this function.
In fact, the only place this function is used (in osdmap_show()),
the non-static buffer was not initialized before the call. (It
might happen to work because the same stack space was getting
reused each time through the loop. Eeeeew!)
This is just an awful couple of functions.
> And, the function of ceph_osdmap_state_str() is to return the osdmap state, so
> it should return "doesn't exist" when all the conditions are not satisfied. I
> fix it in this patch.
>
> Based on C11, snprintf() does nothing if n==0:
> "If n is zero, nothing is written, and s may be a
> null pointer. Otherwise, output characters beyond
> the n-1st are discarded rather than being written to
> the array, and a null character is written at the
> end of the characters actually written into the
> array."
> so I remove the unnecessary check of len (because it is not a busy path and
> saves a few lines of code).
True. But since you know it's not going to do anything why
not only make the call if len is non-zero? I.e.:
else if (len)
snprintf(str, len, "doesn't exist");
With your permission I'll make this change and will commit
this for you. OK?
> Signed-off-by: Cong Ding <dinggnu@gmail.com>
Reviewed-by: Alex Elder <elder@inktank.com>
> ---
> net/ceph/osdmap.c | 27 ++++++++-------------------
> 1 file changed, 8 insertions(+), 19 deletions(-)
>
> diff --git a/net/ceph/osdmap.c b/net/ceph/osdmap.c
> index de73214..3131a99d3 100644
> --- a/net/ceph/osdmap.c
> +++ b/net/ceph/osdmap.c
> @@ -13,26 +13,15 @@
>
> char *ceph_osdmap_state_str(char *str, int len, int state)
> {
> - int flag = 0;
> -
> - if (!len)
> - goto done;
> -
> - *str = '\0';
> - if (state) {
> - if (state & CEPH_OSD_EXISTS) {
> - snprintf(str, len, "exists");
> - flag = 1;
> - }
> - if (state & CEPH_OSD_UP) {
> - snprintf(str, len, "%s%s%s", str, (flag ? ", " : ""),
> - "up");
> - flag = 1;
> - }
> - } else {
> + if ((state & CEPH_OSD_EXISTS) && (state & CEPH_OSD_UP))
> + snprintf(str, len, "exists, up");
> + else if (state & CEPH_OSD_EXISTS)
> + snprintf(str, len, "exists");
> + else if (state & CEPH_OSD_UP)
> + snprintf(str, len, "up");
> + else
> snprintf(str, len, "doesn't exist");
> - }
> -done:
> +
> return str;
> }
>
>
^ permalink raw reply
* Re: Unix Socket buffer attribution
From: Eric Dumazet @ 2013-01-23 16:56 UTC (permalink / raw)
To: Yannick Koehler; +Cc: Cong Wang, netdev
In-Reply-To: <CAJ4BwwG-fimMeSD_DSNZZz=Vjz3Qk_58qEnMGv_gmn0rTSeZAg@mail.gmail.com>
On Wed, 2013-01-23 at 11:36 -0500, Yannick Koehler wrote:
> > So what prevents a malicious program to DOS the machine ?
>
> The recv queue (checked with recvq_full()) and receiving's socket
> rcvbuf (check added in my patch).
>
Nope. The limit is given in number of messages, and its the socket
backlog. Many machines setup a somaxconn = 1024 limit in order
to reasonably listen for TCP connections.
> Actually the current situation can easily lead to a DOS situation. I
> simply have to write one application that connect to a unix socket
> domain and have it send me data for which I never call recvfrom() and
> voilà, all other consumer of this unix socket application will no more
> be able to communicate with this application once it maxed out it's
> sndbuf, default is 128k I believe.
A single message can consume ~128k.
If we allow 1024 messages being sent, we consume 128 Mbytes per evil
socket.
Enough to kill many linux based devices.
You'll have to add proper limits (SO_RCVBUF), accounting the truesize of
all accumulated messages.
^ permalink raw reply
* Linux 3.6.x kernel crash in tcp_slow_start / bictcp_cong_avoid with wfica
From: Pasi Kärkkäinen @ 2013-01-23 16:12 UTC (permalink / raw)
To: netdev
Hello,
My laptop running Fedora 17 randomly crashes when using wfica (Citrix Receiver / ICA Client)
proprietary closed source client for accessing remote desktop server.
wfica is running as a normal user, so it shouldn't be able to cause a kernel crash.
Crashes seem to happen most often when I use either wlan- or 3G mobile data Internet connection.
These kernel crashes happen ramdomly, often 1-2 times a week.
What usually happens is that I notice the Internet connection has died,
and when I check the kernel dmesg I'm seeing the traceback.
At this point I'm still able to use the gnome desktop for maybe 1-5 minutes,
but applications start to fail one by one, and finally everything just halts
and I need to power cycle the laptop.
Without wfica the system is perfectly stable. I've been running memtest86+ for 2 days without errors.
I've seen these crashes happening with multiple (all?) Fedora 17 Linux 3.6.x kernel versions.
Currently I'm running Linux 3.6.11-5.fc17.x86_64. Laptop is Lenovo T430.
I reported this crash also to Fedora 17 bugzilla:
https://bugzilla.redhat.com/show_bug.cgi?id=903273
Kernel crash traceback:
[11056.124003] BUG: soft lockup - CPU#2 stuck for 22s! [wfica:2232]
[11056.124008] Modules linked in: xts gf128mul dm_crypt fuse lockd sunrpc rfcomm bnep ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 nf_conntrack_ipv4 nf_defrag_ipv4 ip6table_filter xt_state nf_conntrack ip6_tables btusb bluetooth arc4 snd_hda_codec_realtek iwldvm uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_core videodev cdc_acm snd_hda_intel snd_hda_codec snd_hwdep mac80211 media snd_seq snd_seq_device snd_pcm thinkpad_acpi snd_page_alloc coretemp snd_timer kvm_intel kvm e1000e iwlwifi cdc_wdm cdc_ncm usbnet mii iTCO_wdt iTCO_vendor_support mei lpc_ich snd mfd_core cfg80211 i2c_i801 soundcore rfkill microcode uinput crc32c_intel ghash_clmulni_intel sdhci_pci sdhci mmc_core wmi i915 video i2c_algo_bit drm_kms_helper drm i2c_core
[11056.124078] CPU 2
[11056.124083] Pid: 2232, comm: wfica Not tainted 3.6.11-1.fc17.x86_64 #1 LENOVO 2349H2G/2349H2G
[11056.124086] RIP: 0010:[<ffffffff8156d3a0>] [<ffffffff8156d3a0>] tcp_slow_start+0x70/0xa0
[11056.124097] RSP: 0018:ffff8802c384ba18 EFLAGS: 00200246
[11056.124099] RAX: 0000000000000000 RBX: ffffffff8106c05b RCX: 000000003db56a41
[11056.124102] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8802c394c600
[11056.124104] RBP: ffff8802c384ba18 R08: 000000000000050e R09: 0000000000000000
[11056.124106] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88030cd98000
[11056.124108] R13: ffff8802c394c978 R14: ffff88030cd98000 R15: 0000000000013cc0
[11056.124111] FS: 0000000000000000(0000) GS:ffff88031e280000(0063) knlGS:00000000f6314b40
[11056.124113] CS: 0010 DS: 002b ES: 002b CR0: 000000008005003b
[11056.124115] CR2: 00007fbf0e912000 CR3: 00000002c3b5b000 CR4: 00000000001407e0
[11056.124117] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[11056.124120] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[11056.124122] Process wfica (pid: 2232, threadinfo ffff8802c384a000, task ffff8802c9d85c40)
[11056.124124] Stack:
[11056.124126] ffff8802c384ba48 ffffffff8159624b ffff8802c384ba48 0000000000000006
[11056.124131] 0000000000000004 0000000000000003 ffff8802c384bb08 ffffffff8155f5e2
[11056.124135] ffff88031e293cc0 ffff88020000050e 00000000e4434980 0000000000000000
[11056.124139] Call Trace:
[11056.124148] [<ffffffff8159624b>] bictcp_cong_avoid+0x5b/0x3c0
[11056.124153] [<ffffffff8155f5e2>] tcp_ack+0x572/0x1210
[11056.124158] [<ffffffff815604fe>] tcp_rcv_established+0x27e/0x8f0
[11056.124163] [<ffffffff8156a584>] tcp_v4_do_rcv+0x1b4/0x4c0
[11056.124170] [<ffffffff81552bc7>] tcp_prequeue_process+0x67/0xb0
[11056.124174] [<ffffffff815576d7>] tcp_recvmsg+0x9d7/0xd80
[11056.124179] [<ffffffff8157cd7b>] inet_recvmsg+0x6b/0x80
[11056.124186] [<ffffffff814f9ed2>] sock_aio_read.part.10+0x142/0x170
[11056.124193] [<ffffffff8108f05c>] ? ttwu_do_wakeup+0x2c/0xf0
[11056.124197] [<ffffffff814f9f25>] sock_aio_read+0x25/0x40
[11056.124204] [<ffffffff8118fa77>] do_sync_read+0xa7/0xe0
[11056.124210] [<ffffffff8119044d>] vfs_read+0x15d/0x180
[11056.124214] [<ffffffff811904ba>] sys_read+0x4a/0x90
[11056.124220] [<ffffffff816286e6>] sysenter_dispatch+0x7/0x21
[11056.124222] Code: 01 f6 39 b7 f0 05 00 00 0f 43 c1 03 87 b8 05 00 00 31 c9 c7 87 f0 05 00 00 00 00 00 00 39 d0 89 87 b8 05 00 00 72 13 0f 1f 40 00 <29> d0 83 c1 01 39 d0 73 f7 89 87 b8 05 00 00 8b 87 bc 05 00 00
Any ideas?
Thanks,
-- Pasi
^ permalink raw reply
* Re: Unix Socket buffer attribution
From: Eric Dumazet @ 2013-01-23 17:13 UTC (permalink / raw)
To: Yannick Koehler; +Cc: Cong Wang, netdev
In-Reply-To: <1358960188.12374.830.camel@edumazet-glaptop>
On Wed, 2013-01-23 at 08:56 -0800, Eric Dumazet wrote:
> You'll have to add proper limits (SO_RCVBUF), accounting the truesize of
> all accumulated messages.
And if you claim being able to remove DOS attacks, you'll also have to
add global limits, at a very minimum.
(a la /proc/sys/net/ipv4/tcp_mem or /proc/sys/net/ipv4/udp_mem)
Its not an easy problem, unfortunately.
^ permalink raw reply
* Re: Unix Socket buffer attribution
From: Yannick Koehler @ 2013-01-23 17:36 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Cong Wang, netdev
In-Reply-To: <1358961185.12374.853.camel@edumazet-glaptop>
Hi Eric,
I am not sure to follow you. I am not changing how sockets works.
I am actually making the af_unix socket works like others, by using
the sndbuf/rcvbuf limits. The code I added was took from netlink.c
and sock.c (sock_queue_err_skb). And actually, I am simply "adding" a
limit check, not removing. The only thing this may do as a negative
side effect is allow more buffer at the same time in the system, but
the global number of buffer remains checked, as it was, if it was,
since I am not changing how buffer gets allocated, just accounted.
Please check my patch.
2013/1/23 Eric Dumazet <eric.dumazet@gmail.com>:
> On Wed, 2013-01-23 at 08:56 -0800, Eric Dumazet wrote:
>
>> You'll have to add proper limits (SO_RCVBUF), accounting the truesize of
>> all accumulated messages.
>
> And if you claim being able to remove DOS attacks, you'll also have to
> add global limits, at a very minimum.
>
> (a la /proc/sys/net/ipv4/tcp_mem or /proc/sys/net/ipv4/udp_mem)
>
> Its not an easy problem, unfortunately.
>
>
>
--
Yannick Koehler
Courriel: yannick@koehler.name
Blog: http://corbeillepensees.blogspot.com
^ permalink raw reply
* Re: [PATCH] net/ceph/osdmap.c: fix undefined behavior when using snprintf()
From: Cong Ding @ 2013-01-23 17:41 UTC (permalink / raw)
To: Alex Elder; +Cc: Sage Weil, David S. Miller, ceph-devel, netdev, linux-kernel
In-Reply-To: <51001447.3030600@inktank.com>
On Wed, Jan 23, 2013 at 10:48:07AM -0600, Alex Elder wrote:
> On 01/22/2013 01:20 PM, Cong Ding wrote:
> > The variable "str" is used as both the source and destination in function
> > snprintf(), which is undefined behavior based on C11. The original description
> > in C11 is:
> > "If copying takes place between objects that
> > overlap, the behavior is undefined."
>
> Yes, this was an ill-advised thing to do in this function.
>
> In fact, the only place this function is used (in osdmap_show()),
> the non-static buffer was not initialized before the call. (It
> might happen to work because the same stack space was getting
> reused each time through the loop. Eeeeew!)
>
> This is just an awful couple of functions.
>
> > And, the function of ceph_osdmap_state_str() is to return the osdmap state, so
> > it should return "doesn't exist" when all the conditions are not satisfied. I
> > fix it in this patch.
> >
> > Based on C11, snprintf() does nothing if n==0:
> > "If n is zero, nothing is written, and s may be a
> > null pointer. Otherwise, output characters beyond
> > the n-1st are discarded rather than being written to
> > the array, and a null character is written at the
> > end of the characters actually written into the
> > array."
> > so I remove the unnecessary check of len (because it is not a busy path and
> > saves a few lines of code).
>
> True. But since you know it's not going to do anything why
> not only make the call if len is non-zero? I.e.:
>
> else if (len)
> snprintf(str, len, "doesn't exist");
>
> With your permission I'll make this change and will commit
> this for you. OK?
It's fine, thanks. But I think it's better to check len in the beginning
because other conditions also call snprintf with parameter len. Like this:
if (!len)
return str;
if ((state & CEPH_OSD_EXISTS) && (state & CEPH_OSD_UP))
snprintf(str, len, "exists, up");
else if (state & CEPH_OSD_EXISTS)
snprintf(str, len, "exists");
else if (state & CEPH_OSD_UP)
snprintf(str, len, "up");
else
snprintf(str, len, "doesn't exist");
return str;
or like this:
if (len) {
if ((state & CEPH_OSD_EXISTS) && (state & CEPH_OSD_UP))
snprintf(str, len, "exists, up");
else if (state & CEPH_OSD_EXISTS)
snprintf(str, len, "exists");
else if (state & CEPH_OSD_UP)
snprintf(str, len, "up");
else
snprintf(str, len, "doesn't exist");
}
return str;
Thanks,
- cong
^ permalink raw reply
* Re: [PATCH] net/ceph/osdmap.c: fix undefined behavior when using snprintf()
From: Alex Elder @ 2013-01-23 17:47 UTC (permalink / raw)
To: Cong Ding; +Cc: Sage Weil, David S. Miller, ceph-devel, netdev, linux-kernel
In-Reply-To: <20130123174150.GA26336@gmail.com>
On 01/23/2013 11:41 AM, Cong Ding wrote:
> On Wed, Jan 23, 2013 at 10:48:07AM -0600, Alex Elder wrote:
>> On 01/22/2013 01:20 PM, Cong Ding wrote:
>>> The variable "str" is used as both the source and destination in function
>>> snprintf(), which is undefined behavior based on C11. The original description
>>> in C11 is:
>>> "If copying takes place between objects that
>>> overlap, the behavior is undefined."
>>
>> Yes, this was an ill-advised thing to do in this function.
>>
>> In fact, the only place this function is used (in osdmap_show()),
>> the non-static buffer was not initialized before the call. (It
>> might happen to work because the same stack space was getting
>> reused each time through the loop. Eeeeew!)
>>
>> This is just an awful couple of functions.
>>
>>> And, the function of ceph_osdmap_state_str() is to return the osdmap state, so
>>> it should return "doesn't exist" when all the conditions are not satisfied. I
>>> fix it in this patch.
>>>
>>> Based on C11, snprintf() does nothing if n==0:
>>> "If n is zero, nothing is written, and s may be a
>>> null pointer. Otherwise, output characters beyond
>>> the n-1st are discarded rather than being written to
>>> the array, and a null character is written at the
>>> end of the characters actually written into the
>>> array."
>>> so I remove the unnecessary check of len (because it is not a busy path and
>>> saves a few lines of code).
>>
>> True. But since you know it's not going to do anything why
>> not only make the call if len is non-zero? I.e.:
>>
>> else if (len)
>> snprintf(str, len, "doesn't exist");
>>
>> With your permission I'll make this change and will commit
>> this for you. OK?
> It's fine, thanks. But I think it's better to check len in the beginning
> because other conditions also call snprintf with parameter len. Like this:
OK. I'll do this. Thank you. -Alex
> if (!len)
> return str;
>
> if ((state & CEPH_OSD_EXISTS) && (state & CEPH_OSD_UP))
> snprintf(str, len, "exists, up");
> else if (state & CEPH_OSD_EXISTS)
> snprintf(str, len, "exists");
> else if (state & CEPH_OSD_UP)
> snprintf(str, len, "up");
> else
> snprintf(str, len, "doesn't exist");
>
> return str;
>
> or like this:
>
> if (len) {
> if ((state & CEPH_OSD_EXISTS) && (state & CEPH_OSD_UP))
> snprintf(str, len, "exists, up");
> else if (state & CEPH_OSD_EXISTS)
> snprintf(str, len, "exists");
> else if (state & CEPH_OSD_UP)
> snprintf(str, len, "up");
> else
> snprintf(str, len, "doesn't exist");
> }
> return str;
>
> Thanks,
> - cong
>
^ permalink raw reply
* Re: 3.7.3+: Bad paging request in ip_rcv_finish while running NFS traffic.
From: Ben Greear @ 2013-01-23 18:15 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1358948110.12374.748.camel@edumazet-glaptop>
On 01/23/2013 05:35 AM, Eric Dumazet wrote:
> On Tue, 2013-01-22 at 23:14 -0800, Ben Greear wrote:
>
>> How are the non-ref-counted dst objects used safely? Any chance
>> that tearing down the IP protocol on a device (or deleting a device)
>> could delete a dst that is referenced by an skb (and thus crashes as
>> I see)?
>
> There is probably a bug. Normally it should be RCU protected.
I'm building net-next, and will do some testing on it later today
if all goes well.
In the meantime, a few questions about RCU.
If the non-ref-counted dst is protected by RCU, does that mean
that from the time we acquire the pointer (ie, set it in the skb),
to the time we clear the pointer from the skb, we must be under
RCU read lock?
If so, that means that ip_rcv_finish must be called under RCU
read lock, etc?
Btw, this seems non related to NFS, so I'm dropping them
from CC after this email unless I hear a request otherwise.
Thanks,
Ben
--
Ben Greear <greearb-my8/4N5VtI7c+919tysfdA@public.gmane.org>
Candela Technologies Inc http://www.candelatech.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
* Re: [PATCH net-next] via-rhine: add 64bit statistics.
From: David Miller @ 2013-01-23 18:25 UTC (permalink / raw)
To: jamie.gloudon; +Cc: netdev, eric.dumazet
In-Reply-To: <510004a3.2444ec0a.1ba7.ffff98b3@mx.google.com>
Please don't submit new versions of patchs using a reply like this,
that makes for a lot of work the subsystem maintainer has to do
to edit the reply to get the commit message to come out right.
Instead, make a fresh new patch posting on the list so that everything
comes out right and your intentions are %100 clear.
Thanks.
^ permalink raw reply
* Re: [PATCH] [RFC] tcp: add ability to set a timestamp offset (v2)
From: David Miller @ 2013-01-23 18:27 UTC (permalink / raw)
To: kuznet
Cc: avagin, netdev, criu, linux-kernel, jmorris, yoshfuji, kaber,
edumazet, ycheng, ncardwell, xemul, davej, mtk.manpages
In-Reply-To: <20130123162025.GA2147@ms2.inr.ac.ru>
From: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
Date: Wed, 23 Jan 2013 20:20:25 +0400
> Hello!
>
> On Wed, Jan 23, 2013 at 07:01:52PM +0400, Andrey Vagin wrote:
>> -#define tcp_time_stamp ((__u32)(jiffies))
>> +#define tcp_time_stamp(tp) ((__u32)(jiffies) + tp->tsoffset)
>
> This implies that you always have some tp in hands. AFAIK this is not true,
> so that I am puzzled how you were able to make something compilable with this definition.
>
> At least in tcp_v4_send_ack() you obviously use some invalid tp, there is
> no socket in hands there. The thing which is called "sk" is dummy.
>
> With container migration I used per-container offset. When you have no container,
> you have to do something a little bit smarter.
I'm starting to have almost no confidence in this change, and even the
basic conceptual idea behind it.
Please go back to the drawing board and really analyze this thing
completely rather than just bouncing broken patches to the list every
day.
Thanks.
^ permalink raw reply
* Re: Unix Socket buffer attribution
From: Hannes Frederic Sowa @ 2013-01-23 18:35 UTC (permalink / raw)
To: Yannick Koehler; +Cc: netdev
In-Reply-To: <CAJ4BwwFYKYOJoP34cGqWyE_AzxMwDNqFG-XDjLB6ZdO4nQmY4w@mail.gmail.com>
On Wed, Jan 23, 2013 at 11:41:16AM -0500, Yannick Koehler wrote:
> I did some more research, I found out that netlink and
> sock_queue_err_skb does the same trick that I claim to be missing
> under the net/unix/af_unix.c. After adding the code, I got a problem
> since af_unix.c "_recvmsg()" functions assume that the skb->sk is
> holding the peer socket not the current one related to the receive
> skb. It extract the sun_path name from it. Since with UDP each
> packet may have a different peer, the only solution I found was to use
> the skb control block to hold the peer name. The problem is that this
> cb member is 48 bytes in length and sun_path is 108 bytes in size. So
> I had to increase it from 48 to 160 bytes.
>
> This obviously increase the cost of an SKB struct, so I do not really
> like this solution. But at least it seems to prove my point and now,
> with this I can have my clients working except the one mis-behaving.
> I am attaching a patch.
Yes, you cannot do that. I would try to place a refcounted pointer to the
original sk in unix_skb_params. Perhaps a refcounted unix_address will dot the
trick, too.
^ permalink raw reply
* [PATCH net-next] drivers/net/ethernet/micrel/ks8851_mll: Implement basic statistics
From: Choi, David @ 2013-01-23 18:46 UTC (permalink / raw)
To: netdev@vger.kernel.org
Cc: Doong, Ping, Choi, David, davem@davemloft.net, joe@perches.com,
bhutchings@solarflare.com
From: David J. Choi <david.choi@micrel.com>
Summary of changes:
add codes to collect basic statistical information about Ethernet packets.
Signed-off-by: David J. Choi <david.choi@micrel.com>
---
--- net-next/drivers/net/ethernet/micrel/ks8851_mll.c.orig 2013-01-22 17:25:59.000000000 -0800
+++ net-next/drivers/net/ethernet/micrel/ks8851_mll.c 2013-01-23 10:31:24.000000000 -0800
@@ -793,17 +793,35 @@ static void ks_rcv(struct ks_net *ks, st
frame_hdr = ks->frame_head_info;
while (ks->frame_cnt--) {
skb = netdev_alloc_skb(netdev, frame_hdr->len + 16);
- if (likely(skb && (frame_hdr->sts & RXFSHR_RXFV) &&
+ if (unlikely(!skb)) {
+ /* discard the packet from the device */
+ ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
+ netdev->stats.rx_dropped++;
+ }
+
+ else if (likely((frame_hdr->sts & RXFSHR_RXFV) &&
(frame_hdr->len < RX_BUF_SIZE) && frame_hdr->len)) {
skb_reserve(skb, 2);
/* read data block including CRC 4 bytes */
ks_read_qmu(ks, (u16 *)skb->data, frame_hdr->len);
- skb_put(skb, frame_hdr->len);
+
+ /* exclude the size of CRC */
+ skb_put(skb, frame_hdr->len - 4);
skb->protocol = eth_type_trans(skb, netdev);
netif_rx(skb);
+ netdev->stats.rx_packets++;
+
+ /* crc field */
+ netdev->stats.rx_bytes += frame_hdr->len - 4;
} else {
- pr_err("%s: err:skb alloc\n", __func__);
- ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
+ /* discard the packet from the device */
+ ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
+ netdev->stats.rx_dropped++;
+ if (frame_hdr->len >= RX_BUF_SIZE || !frame_hdr->len)
+ netdev->stats.rx_length_errors++;
+ if (!(frame_hdr->sts & RXFSHR_RXFV))
+ netdev->stats.rx_frame_errors++;
+
if (skb)
dev_kfree_skb_irq(skb);
}
@@ -876,6 +894,8 @@ static irqreturn_t ks_irq(int irq, void
pmecr &= ~PMECR_WKEVT_MASK;
ks_wrreg16(ks, KS_PMECR, pmecr | PMECR_WKEVT_LINK);
}
+ if (unlikely(status & IRQ_RXOI))
+ ks->netdev->stats.rx_over_errors++;
/* this should be the last in IRQ handler*/
ks_restore_cmd_reg(ks);
@@ -1015,6 +1035,8 @@ static int ks_start_xmit(struct sk_buff
if (likely(ks_tx_fifo_space(ks) >= skb->len + 12)) {
ks_write_qmu(ks, skb->data, skb->len);
+ netdev->stats.tx_bytes += skb->len;
+ netdev->stats.tx_packets++;
dev_kfree_skb(skb);
} else
retv = NETDEV_TX_BUSY;
---
^ permalink raw reply
* Re: [PATCH net] net: cdc_mbim: send ZLP only for the specific buggy device
From: David Miller @ 2013-01-23 18:47 UTC (permalink / raw)
To: bjorn; +Cc: netdev, linux-usb, oneukum, gsuarez, alexey.orishko, y.kaliuta
In-Reply-To: <1358938622-18344-1-git-send-email-bjorn@mork.no>
From: Bjørn Mork <bjorn@mork.no>
Date: Wed, 23 Jan 2013 11:57:02 +0100
> Reverting 328d7b8 and instead adding an exception for the
> Sierra Wireless MC7710.
>
> commit 328d7b8 (net: cdc_mbim: send ZLP after max sized NTBs)
> added a workaround for an issue observed on one specific device.
> Concerns were raised that this workaround adds a performance
> penalty to all devices based on questionable, if not buggy,
> behaviour of a single device:
>
> "If you add ZLP for NTBs of dwNtbOutMaxSize, you are heavily affecting CPU
> load, increasing interrupt load by factor of 2 in high load traffic
> scenario and possibly decreasing throughput for all other devices
> which behaves correctly."
>
> "The idea of NCM was to avoid extra ZLPs. If your transfer is exactly
> dwNtbOutMaxSize, it's known, you can submit such request on the receiver
> side and you do not need any EOT indicatation, so the frametime can be
> used for useful data."
>
> Adding a device specific exception to prevent the workaround from
> affecting well behaved devices.
>
> The assumption here is that needing a ZLP is truly an *exception*.
> We do not yet have enough data to verify this. The generic
> workaround in commit 328d7b8 should be considered acceptable despite
> the performance penalty if the exception list becomes a maintainance
> hassle.
>
> Cc: Alexey ORISHKO <alexey.orishko@stericsson.com>
> Cc: Yauheni Kaliuta <y.kaliuta@gmail.com>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
Applied.
^ permalink raw reply
* Re: [PATCH 1/2] tuntap: reduce memory using of queues
From: David Miller @ 2013-01-23 18:47 UTC (permalink / raw)
To: jasowang; +Cc: mst, netdev, linux-kernel, eric.dumazet, dwmw2
In-Reply-To: <1358949553-30309-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 23 Jan 2013 21:59:12 +0800
> A MAX_TAP_QUEUES(1024) queues of tuntap device is always allocated
> unconditionally even userspace only requires a single queue device. This is
> unnecessary and will lead a very high order of page allocation when has a high
> possibility to fail. Solving this by creating a one queue net device when
> userspace only use one queue and also reduce MAX_TAP_QUEUES to
> DEFAULT_MAX_NUM_RSS_QUEUES which can guarantee the success of
> the allocation.
>
> Reported-by: Dirk Hohndel <dirk@hohndel.org>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH 2/2] tuntap: limit the number of flow caches
From: David Miller @ 2013-01-23 18:50 UTC (permalink / raw)
To: jasowang; +Cc: mst, netdev, linux-kernel, stephen
In-Reply-To: <1358949553-30309-2-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Wed, 23 Jan 2013 21:59:13 +0800
> We create new flow caches when a new flow is identified by tuntap, This may lead
> some issues:
>
> - userspace may produce a huge amount of short live flows to exhaust host memory
> - the unlimited number of flow caches may produce a long list which increase the
> time in the linear searching
>
> Solve this by introducing a limit of total number of flow caches.
>
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
Applied, but really flow caches are an extremely broken concept especially
when external entities control the population of such caches.
We removed the routing cache of the ipv4 networking code exactly because
this kind of crap does not work at all.
Next you're going to have to add a delicately managed garbage
collection scheme for this tuntap flow cache, and that will be tuned
endlessly, when the real issue is that fundamentally this does not
work.
Instead, make the full lookup scale properly and use appropriate data
structures. It won't be as fast as a simple hash table demux, but
it'll actually be immune to growth issues and DoS attacks and give
consistent and repeatable lookup performance regardless of traffic
patterns.
^ permalink raw reply
* Re: [PATCH net] r8169: remove the obsolete and incorrect AMD workaround
From: David Miller @ 2013-01-23 18:52 UTC (permalink / raw)
To: romieu; +Cc: timo.teras, netdev
In-Reply-To: <20130123000541.GA9515@electric-eye.fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed, 23 Jan 2013 01:05:41 +0100
> Timo Teräs <timo.teras@iki.fi> :
> [...]
>
> Acked-by: Francois Romieu <romieu@fr.zoreil.com>
No reply, so I am just going to assume you wanted me to integrate
this directly, so I've done so.
^ permalink raw reply
* Re: [PATCH][net-next] gianfar: Restore promisc mode on gfar_init_mac()
From: David Miller @ 2013-01-23 18:54 UTC (permalink / raw)
To: claudiu.manoil; +Cc: netdev, cedric.voncken
In-Reply-To: <1358936316-8195-1-git-send-email-claudiu.manoil@freescale.com>
From: Claudiu Manoil <claudiu.manoil@freescale.com>
Date: Wed, 23 Jan 2013 12:18:36 +0200
> Reactivate promiscuous mode in H/W upon gfar_init_mac(), if the
> net dev requires it (IFF_PROMISC flag set).
> This way the promisc mode is preserved accross device reset conditions
> like tx timeout, device restore, a.s.o.
>
> Signed-off-by: Voncken C Acksys <cedric.voncken@acksys.fr>
> Signed-off-by: Claudiu Manoil <claudiu.manoil@freescale.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Tejun Heo @ 2013-01-23 18:55 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: tglx, peterz, oleg, paulmck, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130122073347.13822.85876.stgit@srivatsabhat.in.ibm.com>
Hello, Srivatsa.
First of all, I'm not sure whether we need to be this step-by-step
when introducing something new. It's not like we're transforming an
existing implementation and it doesn't seem to help understanding the
series that much either.
On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
> lock-ordering related problems (unlike per-cpu locks). However, global
So, unfortunately, this already seems broken, right? The problem here
seems to be that previously, say, read_lock() implied
preempt_disable() but as this series aims to move away from it, it
introduces the problem of locking order between such locks and the new
contruct.
The only two options are either punishing writers or identifying and
updating all such possible deadlocks. percpu_rwsem does the former,
right? I don't know how feasible the latter would be. Srivatsa,
you've been looking at all the places which would require conversion,
how difficult would doing the latter be?
> +#define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
> + (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
> +
> +#define reader_nested_percpu(pcpu_rwlock) \
> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
> +
> +#define writer_active(pcpu_rwlock) \
> + (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
Why are these in the public header file? Are they gonna be used to
inline something?
> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
> + unsigned int cpu)
> +{
> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
> +}
> +
> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
> + unsigned int cpu)
> +{
> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
> +}
> +
> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
> +{
> + unsigned int cpu;
> +
> + for_each_online_cpu(cpu)
> + raise_writer_signal(pcpu_rwlock, cpu);
> +
> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
> +}
> +
> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
> +{
> + unsigned int cpu;
> +
> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
> +
> + for_each_online_cpu(cpu)
> + drop_writer_signal(pcpu_rwlock, cpu);
> +
> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
> +}
It could be just personal preference but I find the above one line
wrappers more obfuscating than anything else. What's the point of
wrapping writer_signal = true/false into a separate function? These
simple wrappers just add layers that people have to dig through to
figure out what's going on without adding anything of value. I'd much
prefer collapsing these into the percpu_write_[un]lock().
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH net-next 0/12] bnx2x: patch series
From: David Miller @ 2013-01-23 18:58 UTC (permalink / raw)
To: yuvalmin; +Cc: netdev, eilong, ariele
In-Reply-To: <1358947314-3851-1-git-send-email-yuvalmin@broadcom.com>
From: "Yuval Mintz" <yuvalmin@broadcom.com>
Date: Wed, 23 Jan 2013 15:21:42 +0200
> Hi Dave,
>
> This patch series contain several Semi-semantic patches (i.e., some are purely
> semantic, such as correcting comments & typos, while others slightly change the
> bnx2x code without effectively changing any flow), while other fix several small
> issues, mostly either memory-related or missing segments from our recent SR-IOV
> submission.
>
> Please consider applying this patch-series to 'net-next'.
Series applied, thanks.
^ permalink raw reply
* Re: pull request (net-next): ipsec-next 2013-01-23
From: David Miller @ 2013-01-23 19:00 UTC (permalink / raw)
To: steffen.klassert; +Cc: herbert, netdev
In-Reply-To: <1358929144-17806-1-git-send-email-steffen.klassert@secunet.com>
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Wed, 23 Jan 2013 09:18:56 +0100
> 1) Add a statistic counter for invalid output states and
> remove a superfluous state valid check, from Li RongQing.
>
> 2) Probe for asynchronous block ciphers instead of synchronous block
> ciphers to make the asynchronous variants available even if no
> synchronous block ciphers are found, from Jussi Kivilinna.
>
> 3) Make rfc3686 asynchronous block cipher and make use of
> the new asynchronous variant, from Jussi Kivilinna.
>
> 4) Replace some rwlocks by rcu, from Cong Wang.
>
> 5) Remove some unused defines.
>
> Please pull or let me know if there are problems.
Pulled, thanks Steffen.
I hope Herbert is ok with the small crypto bits going in this way,
since they really are IPSEC specific.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] drivers/net/ethernet/micrel/ks8851_mll: Implement basic statistics
From: Joe Perches @ 2013-01-23 19:09 UTC (permalink / raw)
To: Choi, David
Cc: netdev@vger.kernel.org, Doong, Ping, davem@davemloft.net,
bhutchings@solarflare.com
In-Reply-To: <FD9AD8C5375B924CABC56D982DB3A802079D33CE@EXMB1.micrel.com>
On Wed, 2013-01-23 at 18:46 +0000, Choi, David wrote:
> +++ net-next/drivers/net/ethernet/micrel/ks8851_mll.c 2013-01-23 10:31:24.000000000 -0800
> @@ -793,17 +793,35 @@ static void ks_rcv(struct ks_net *ks, st
> frame_hdr = ks->frame_head_info;
> while (ks->frame_cnt--) {
> skb = netdev_alloc_skb(netdev, frame_hdr->len + 16);
> - if (likely(skb && (frame_hdr->sts & RXFSHR_RXFV) &&
> + if (unlikely(!skb)) {
> + /* discard the packet from the device */
> + ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
> + netdev->stats.rx_dropped++;
> + }
> +
> + else if (likely((frame_hdr->sts & RXFSHR_RXFV) &&
Couple of trivial comments:
it' be nicer if the close brace and else
was on the same line
} else if (likely(...
> (frame_hdr->len < RX_BUF_SIZE) && frame_hdr->len)) {
> skb_reserve(skb, 2);
> /* read data block including CRC 4 bytes */
> ks_read_qmu(ks, (u16 *)skb->data, frame_hdr->len);
> - skb_put(skb, frame_hdr->len);
> +
> + /* exclude the size of CRC */
> + skb_put(skb, frame_hdr->len - 4);
> skb->protocol = eth_type_trans(skb, netdev);
> netif_rx(skb);
> + netdev->stats.rx_packets++;
> +
> + /* crc field */
> + netdev->stats.rx_bytes += frame_hdr->len - 4;
> } else {
> - pr_err("%s: err:skb alloc\n", __func__);
> - ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
> + /* discard the packet from the device */
> + ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
> + netdev->stats.rx_dropped++;
> + if (frame_hdr->len >= RX_BUF_SIZE || !frame_hdr->len)
> + netdev->stats.rx_length_errors++;
> + if (!(frame_hdr->sts & RXFSHR_RXFV))
> + netdev->stats.rx_frame_errors++;
> +
> if (skb)
> dev_kfree_skb_irq(skb);
You don't need to test skb here anymore.
^ permalink raw reply
* Re: Doubts about listen backlog and tcp_max_syn_backlog
From: Rick Jones @ 2013-01-23 19:28 UTC (permalink / raw)
To: Leandro Lucarella; +Cc: Eric Dumazet, netdev, linux-kernel
In-Reply-To: <20130123104736.GK4608@sociomantic.com>
On 01/23/2013 02:47 AM, Leandro Lucarella wrote:
> Thanks for the info. I'm definitely dropping SYNs and sending cookies,
> around 50/s. Is there any way to tell how many connections are queued in
> a particular socket?
I am not familiar with one. Doesn't mean there isn't one, only that I
am not able to think of it.
> Then if syncookies are enabled, the time spent in connect() shouldn't be
> bigger than 3 seconds even if SYNs are being "dropped" by listen, right?
Do you mean if "ESTABLISHED" connections are dropped because the listen
queue is full? I don't think I would put that as "SYNs being dropped by
listen" - too easy to confuse that with an actual dropping of a SYN segment.
But yes, I would not expect a connect() call to remain incomplete for
any longer than it took to receive an SYN|ACK from the other end. That
would be 3 (,9, 21, etc...) seconds on a kernel with 3 seconds as the
initial retransmission timeout.
rick
^ permalink raw reply
* Re: [PATCH v5 04/45] percpu_rwlock: Implement the core design of Per-CPU Reader-Writer Locks
From: Srivatsa S. Bhat @ 2013-01-23 19:33 UTC (permalink / raw)
To: Tejun Heo
Cc: tglx, peterz, oleg, paulmck, rusty, mingo, akpm, namhyung,
rostedt, wangyun, xiaoguangrong, rjw, sbw, fweisbec, linux,
nikunj, linux-pm, linux-arch, linux-arm-kernel, linuxppc-dev,
netdev, linux-doc, linux-kernel
In-Reply-To: <20130123185522.GG2373@mtj.dyndns.org>
On 01/24/2013 12:25 AM, Tejun Heo wrote:
> Hello, Srivatsa.
>
> First of all, I'm not sure whether we need to be this step-by-step
> when introducing something new. It's not like we're transforming an
> existing implementation and it doesn't seem to help understanding the
> series that much either.
>
Hmm.. I split it up into steps to help explain the reasoning behind
the code sufficiently, rather than spring all of the intricacies at
one go (which would make it very hard to write the changelog/comments
also). The split made it easier for me to document it well in the
changelog, because I could deal with reasonable chunks of code/complexity
at a time. IMHO that helps people reading it for the first time to
understand the logic easily.
> On Tue, Jan 22, 2013 at 01:03:53PM +0530, Srivatsa S. Bhat wrote:
>> Using global rwlocks as the backend for per-CPU rwlocks helps us avoid many
>> lock-ordering related problems (unlike per-cpu locks). However, global
>
> So, unfortunately, this already seems broken, right? The problem here
> seems to be that previously, say, read_lock() implied
> preempt_disable() but as this series aims to move away from it, it
> introduces the problem of locking order between such locks and the new
> contruct.
>
Not sure I got your point correctly. Are you referring to Steve's comment
that rwlocks are probably fair now (and hence not really safe when used
like this)? If yes, I haven't actually verified that yet, but yes, that
will make this hard to use, since we need to take care of locking rules.
But suppose rwlocks are unfair (as I had assumed them to be), then we
have absolutely no problems and no lock-ordering to worry about.
> The only two options are either punishing writers or identifying and
> updating all such possible deadlocks. percpu_rwsem does the former,
> right? I don't know how feasible the latter would be.
I don't think we can avoid looking into all the possible deadlocks,
as long as we use rwlocks inside get/put_online_cpus_atomic() (assuming
rwlocks are fair). Even with Oleg's idea of using synchronize_sched()
at the writer, we still need to take care of locking rules, because the
synchronize_sched() only helps avoid the memory barriers at the reader,
and doesn't help get rid of the rwlocks themselves.
So in short, I don't see how we can punish the writers and thereby somehow
avoid looking into possible deadlocks (if rwlocks are fair).
> Srivatsa,
> you've been looking at all the places which would require conversion,
> how difficult would doing the latter be?
>
The problem is that some APIs like smp_call_function() will need to use
get/put_online_cpus_atomic(). That is when the locking becomes tricky
in the subsystem which invokes these APIs with other (subsystem-specific,
internal) locks held. So we could potentially use a convention such as
"Make get/put_online_cpus_atomic() your outer-most calls, within which
you nest the other locks" to rule out all ABBA deadlock possibilities...
But we might still hit some hard-to-convert places..
BTW, Steve, fair rwlocks doesn't mean the following scenario will result
in a deadlock right?
CPU 0 CPU 1
read_lock(&rwlock)
write_lock(&rwlock) //spins, because CPU 0
//has acquired the lock for read
read_lock(&rwlock)
^^^^^
What happens here? Does CPU 0 start spinning (and hence deadlock) or will
it continue realizing that it already holds the rwlock for read?
If the above ends in a deadlock, then its next to impossible to convert
all the places safely (because the above mentioned convention will simply
fall apart).
>> +#define reader_uses_percpu_refcnt(pcpu_rwlock, cpu) \
>> + (ACCESS_ONCE(per_cpu(*((pcpu_rwlock)->reader_refcnt), cpu)))
>> +
>> +#define reader_nested_percpu(pcpu_rwlock) \
>> + (__this_cpu_read(*((pcpu_rwlock)->reader_refcnt)) > 1)
>> +
>> +#define writer_active(pcpu_rwlock) \
>> + (__this_cpu_read(*((pcpu_rwlock)->writer_signal)))
>
> Why are these in the public header file? Are they gonna be used to
> inline something?
>
No, I can put it in the .c file itself. Will do.
>> +static inline void raise_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>> + unsigned int cpu)
>> +{
>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = true;
>> +}
>> +
>> +static inline void drop_writer_signal(struct percpu_rwlock *pcpu_rwlock,
>> + unsigned int cpu)
>> +{
>> + per_cpu(*pcpu_rwlock->writer_signal, cpu) = false;
>> +}
>> +
>> +static void announce_writer_active(struct percpu_rwlock *pcpu_rwlock)
>> +{
>> + unsigned int cpu;
>> +
>> + for_each_online_cpu(cpu)
>> + raise_writer_signal(pcpu_rwlock, cpu);
>> +
>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>> +}
>> +
>> +static void announce_writer_inactive(struct percpu_rwlock *pcpu_rwlock)
>> +{
>> + unsigned int cpu;
>> +
>> + drop_writer_signal(pcpu_rwlock, smp_processor_id());
>> +
>> + for_each_online_cpu(cpu)
>> + drop_writer_signal(pcpu_rwlock, cpu);
>> +
>> + smp_mb(); /* Paired with smp_rmb() in percpu_read_[un]lock() */
>> +}
>
> It could be just personal preference but I find the above one line
> wrappers more obfuscating than anything else. What's the point of
> wrapping writer_signal = true/false into a separate function? These
> simple wrappers just add layers that people have to dig through to
> figure out what's going on without adding anything of value. I'd much
> prefer collapsing these into the percpu_write_[un]lock().
>
Sure, I see your point. I'll change that.
Thanks a lot for your feedback Tejun!
Regards,
Srivatsa S. Bhat
^ permalink raw reply
* RE: [PATCH net-next] drivers/net/ethernet/micrel/ks8851_mll: Implement basic statistics
From: Choi, David @ 2013-01-23 19:36 UTC (permalink / raw)
To: Joe Perches
Cc: netdev@vger.kernel.org, Doong, Ping, davem@davemloft.net,
bhutchings@solarflare.com
In-Reply-To: <1358968194.2107.37.camel@joe-AO722>
From: David J. Choi <david.choi@micrel.com>
Summary of changes:
add codes to collect basic statistical information about Ethernet packets.
Signed-off-by: David J. Choi <david.choi@micrel.com>
---
--- net-next/drivers/net/ethernet/micrel/ks8851_mll.c.orig 2013-01-22 17:25:59.000000000 -0800
+++ net-next/drivers/net/ethernet/micrel/ks8851_mll.c 2013-01-23 11:28:45.000000000 -0800
@@ -793,19 +793,34 @@ static void ks_rcv(struct ks_net *ks, st
frame_hdr = ks->frame_head_info;
while (ks->frame_cnt--) {
skb = netdev_alloc_skb(netdev, frame_hdr->len + 16);
- if (likely(skb && (frame_hdr->sts & RXFSHR_RXFV) &&
+ if (unlikely(!skb)) {
+ /* discard the packet from the device */
+ ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
+ netdev->stats.rx_dropped++;
+ } else if (likely((frame_hdr->sts & RXFSHR_RXFV) &&
(frame_hdr->len < RX_BUF_SIZE) && frame_hdr->len)) {
skb_reserve(skb, 2);
/* read data block including CRC 4 bytes */
ks_read_qmu(ks, (u16 *)skb->data, frame_hdr->len);
- skb_put(skb, frame_hdr->len);
+
+ /* exclude the size of CRC */
+ skb_put(skb, frame_hdr->len - 4);
skb->protocol = eth_type_trans(skb, netdev);
netif_rx(skb);
+ netdev->stats.rx_packets++;
+
+ /* crc field */
+ netdev->stats.rx_bytes += frame_hdr->len - 4;
} else {
- pr_err("%s: err:skb alloc\n", __func__);
- ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
- if (skb)
- dev_kfree_skb_irq(skb);
+ /* discard the packet from the device */
+ ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
+ netdev->stats.rx_dropped++;
+ if (frame_hdr->len >= RX_BUF_SIZE || !frame_hdr->len)
+ netdev->stats.rx_length_errors++;
+ if (!(frame_hdr->sts & RXFSHR_RXFV))
+ netdev->stats.rx_frame_errors++;
+
+ dev_kfree_skb_irq(skb);
}
frame_hdr++;
}
@@ -876,6 +891,8 @@ static irqreturn_t ks_irq(int irq, void
pmecr &= ~PMECR_WKEVT_MASK;
ks_wrreg16(ks, KS_PMECR, pmecr | PMECR_WKEVT_LINK);
}
+ if (unlikely(status & IRQ_RXOI))
+ ks->netdev->stats.rx_over_errors++;
/* this should be the last in IRQ handler*/
ks_restore_cmd_reg(ks);
@@ -1015,6 +1032,8 @@ static int ks_start_xmit(struct sk_buff
if (likely(ks_tx_fifo_space(ks) >= skb->len + 12)) {
ks_write_qmu(ks, skb->data, skb->len);
+ netdev->stats.tx_bytes += skb->len;
+ netdev->stats.tx_packets++;
dev_kfree_skb(skb);
} else
retv = NETDEV_TX_BUSY;
---
^ permalink raw reply
* RE: [PATCH v2 net-next 1/2] be2net: Modify GPL Marking in all source files.
From: Bandi,Sarveshwar @ 2013-01-23 19:37 UTC (permalink / raw)
To: Bandi,Sarveshwar, David Miller; +Cc: netdev@vger.kernel.org, Vasundhara, Volam
In-Reply-To: <32aacb12-c1af-4cff-becb-eb046eef0d6e@CMEXHTCAS2.ad.emulex.com>
Dave,
Does this explanation make sense? Would you want me to resubmit the patches?
Thanks,
Sarvesh
-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Bandi,Sarveshwar
Sent: Wednesday, January 23, 2013 12:58 PM
To: David Miller
Cc: netdev@vger.kernel.org; Vasundhara, Volam
Subject: RE: [PATCH v2 net-next 1/2] be2net: Modify GPL Marking in all source files.
Dave,
Can I assume that the first 3 points are acceptable (copyright being extended, adding EMULEX/SLI as trademarks, and adding website information)?
As for the last one about disclaimer, most driver had it all the time and benet did not. It mentions section 15 "Disclaimer of Warranty" of gpl license explicitly (http://www.gnu.org/licenses/gpl.html).
Is there an issue with this?
Thanks,
Sarvesh
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, January 23, 2013 12:36 PM
To: Bandi,Sarveshwar
Cc: netdev@vger.kernel.org; Vasundhara, Volam
Subject: Re: [PATCH v2 net-next 1/2] be2net: Modify GPL Marking in all source files.
From: <sarveshwar.bandi@emulex.com>
Date: Wed, 23 Jan 2013 12:22:03 +0530
> From: Vasundhara Volam <vasundhara.volam@emulex.com>
>
> The following modifications have been made to copyright:
> - years of copyright extended to include 2013
> - specify EMULEX and SLI as trademarks of Emulex corporation
> - specify emulex website
> - Added a more detailed disclaimer " ALL EXPRESS OR IMPLIED
> CONDITIONS, REPRESENTATIONS AND WARRANTIES, INCLUDING ANY IMPLIED
> WARRANTY OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE, OR
> NON-INFRINGEMENT, ARE DISCLAIMED, EXCEPT TO THE EXTENT THAT SUCH
> DISCLAIMERS ARE HELD TO BE LEGALLY INVALID."
>
> Signed-off-by: Vasundhara Volam <vasundhara.volam@emulex.com>
> Signed-off-by: Sarveshwar Bandi <sarveshwar.bandi@emulex.com>
You're just essentially reading the contents of the patch, that's not satisfactory.
In particular, the problem is that you aren't explaining why you're making all of these changes.
There needs to be justification for copyright changes like this, you are not providing them.
--
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
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox