* Re: WARNING: at net/sched/sch_generic.c:256 dev_watchdog+0x277/0x280()
From: Francois Romieu @ 2012-05-03 6:47 UTC (permalink / raw)
To: Alex Villacís Lasso; +Cc: netdev
In-Reply-To: <4FA1C331.80909@fiec.espol.edu.ec>
(please avoid posting twice)
Alex Villacís Lasso <avillaci@fiec.espol.edu.ec> :
[...]
> I am currently away from the target computer. How should I check for this? lspci?
lspci can not tell much. Use 'dmesg | grep XID' instead.
A complete dmesg would be welcome.
It could help to know a few things :
- does the problem qualify as a regression since some kernel version ?
If so which one ?
- can it be reproduced with a kernel that has not been vbox tainted ?
- does networking recover ?
Thanks.
--
Ueimor
^ permalink raw reply
* Re: [PATCH 3/8] Sometimes the ISDN chip only controls the D-channel
From: David Miller @ 2012-05-03 6:50 UTC (permalink / raw)
To: kkeil; +Cc: netdev
In-Reply-To: <4FA2265D.6040000@linux-pingi.de>
From: Karsten Keil <kkeil@linux-pingi.de>
Date: Thu, 03 May 2012 08:31:57 +0200
> I did put the additional PCM infrastructure in this series, because
> the approval test was done with it in place.
> I did plan the update of the low level drivers in a separate patchset
> from the beginning.
>
> What do you prefer, adding the driver part now, as additional patch, or
> removing this additional infrastruckture part and submit it in a later
> series ?
I feel like I'm talking to a wall.
A patch should do one, and only one thing. It should not have
changes which are unrelated to that one thing.
What part of this is so hard to understand?
To make matters worse, you didn't even make a mention of those
unrelated changes in your commit message.
So that patch was bogus on at least two counts.
^ permalink raw reply
* Re: [PATCH 3/8] Sometimes the ISDN chip only controls the D-channel
From: Karsten Keil @ 2012-05-03 6:31 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20120501.133007.569588994121475807.davem@davemloft.net>
Hello David,
Am 01.05.2012 19:30, schrieb David Miller:
> From: Karsten Keil <kkeil@linux-pingi.de>
> Date: Sat, 28 Apr 2012 13:43:19 +0200
>
>> The B-channels are only accessed via the PCM backplane.
>> Add infrastruckture for this special mode.
>>
>> Signed-off-by: Karsten Keil <kkeil@linux-pingi.de>
>
> I dread reviewing these ISDN patch sets because they are so
> full of problems, and it's so damn obvious how little care is
> put into preparing them.
>
> What I see is that you put the minimum amount of work necessary
> into splitting up your huge ISDN patch set submission into more
> managable pieces, and as a result you are introducing problems.
>
I did put the additional PCM infrastructure in this series, because
the approval test was done with it in place.
I did plan the update of the low level drivers in a separate patchset
from the beginning.
What do you prefer, adding the driver part now, as additional patch, or
removing this additional infrastruckture part and submit it in a later
series ?
>> diff --git a/drivers/isdn/mISDN/socket.c b/drivers/isdn/mISDN/socket.c
>> index abe2d69..502bcf1 100644
>> --- a/drivers/isdn/mISDN/socket.c
>> +++ b/drivers/isdn/mISDN/socket.c
>> @@ -270,6 +270,7 @@ data_sock_release(struct socket *sock)
>> break;
>> case ISDN_P_LAPD_TE:
>> case ISDN_P_LAPD_NT:
>> + case ISDN_P_B_PCM:
>> case ISDN_P_B_RAW:
>> case ISDN_P_B_HDLC:
>> case ISDN_P_B_X75SLP:
>
> Ok, that's fine.
>
>> @@ -148,6 +149,8 @@ struct bchannel {
>> u_int state;
>> void *hw;
>> int slot; /* multiport card channel slot */
>> + int pcm_tx; /* PCM tx slot nr */
>> + int pcm_rx; /* PCM rx slot nr */
>> struct timer_list timer;
>> /* receive data */
>> struct sk_buff *rx_skb;
>
> But what the hell is this? These structure members are unused by
> this patch, and in fact no patch in your entire series uses them.
>
>> @@ -360,8 +360,8 @@ clear_channelmap(u_int nr, u_char *map)
>> #define MISDN_CTRL_LOOP 0x0001
>> #define MISDN_CTRL_CONNECT 0x0002
>> #define MISDN_CTRL_DISCONNECT 0x0004
>> -#define MISDN_CTRL_PCMCONNECT 0x0010
>> -#define MISDN_CTRL_PCMDISCONNECT 0x0020
>> +#define MISDN_CTRL_GET_PCM_SLOTS 0x0010
>> +#define MISDN_CTRL_SET_PCM_SLOTS 0x0020
>> #define MISDN_CTRL_SETPEER 0x0040
>> #define MISDN_CTRL_UNSETPEER 0x0080
>> #define MISDN_CTRL_RX_OFF 0x0100
>
> Another completely unrelated change, nothing in this patch uses
> these new defines.
>
>> @@ -381,6 +381,10 @@ clear_channelmap(u_int nr, u_char *map)
>> #define MISDN_CTRL_HFC_WD_INIT 0x4009
>> #define MISDN_CTRL_HFC_WD_RESET 0x400A
>>
>> +/* special PCM slot numbers */
>> +#define MISDN_PCM_SLOT_DISABLE -1 /* PCM disabled */
>> +#define MISDN_PCM_SLOT_IGNORE -2 /* PCM setting will be not changed */
>> +
>> /* socket options */
>> #define MISDN_TIME_STAMP 0x0001
>>
>
> Same thing.
>
>> @@ -389,6 +393,7 @@ struct mISDN_ctrl_req {
>> int channel;
>> int p1;
>> int p2;
>> + int p3;
>> };
>>
>> /* muxer options */
>
> And again, same problem.
>
> You really need to get your act in gear and prepare your patches
> properly, so that they don't have unrelated changes in them.
>
> This is not amateur hour.
>
>
^ permalink raw reply
* Re: [PATCH] net: skb_set_dev do not unconditionally drop ref to dst
From: David Miller @ 2012-05-03 6:28 UTC (permalink / raw)
To: eric.dumazet; +Cc: blaschka, netdev, arnd, linux-s390
In-Reply-To: <1335941961.22133.48.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 02 May 2012 08:59:21 +0200
> If Arnd doesnt care to even reply, we can just revert his buggy
> patch, instead of trying to understand and fix all issues.
Agreed, it's the best sounding solution by far.
^ permalink raw reply
* Re: linux-next: build failure after merge of the final tree (net-next tree related)
From: David Miller @ 2012-05-03 6:26 UTC (permalink / raw)
To: sfr; +Cc: netdev, linux-next, linux-kernel, edumazet
In-Reply-To: <20120503161623.ad219e088031fd66be212407@canb.auug.org.au>
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 3 May 2012 16:16:23 +1000
> After merging the final tree, today's linux-next build (sparc32 defconfig)
> failed like this:
>
> net/core/sock.c: In function '__release_sock':
> net/core/sock.c:1703:4: error: implicit declaration of function 'prefetch' [-Werror=implicit-function-declaration]
>
> Caused by commit e4cbb02a1070 ("net: add a prefetch in socket backlog
> processing").
>
> I have reverted that commit for today.
Fixed as follows:
--------------------
net: Add missing linux/prefetch.h include to net/core/sock.c
Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/core/sock.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/core/sock.c b/net/core/sock.c
index 1a88351..b8c818e 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -113,6 +113,7 @@
#include <linux/user_namespace.h>
#include <linux/static_key.h>
#include <linux/memcontrol.h>
+#include <linux/prefetch.h>
#include <asm/uaccess.h>
--
1.7.10
^ permalink raw reply related
* linux-next: build failure after merge of the final tree (net-next tree related)
From: Stephen Rothwell @ 2012-05-03 6:16 UTC (permalink / raw)
To: David Miller, netdev; +Cc: linux-next, linux-kernel, Eric Dumazet
[-- Attachment #1: Type: text/plain, Size: 474 bytes --]
Hi all,
After merging the final tree, today's linux-next build (sparc32 defconfig)
failed like this:
net/core/sock.c: In function '__release_sock':
net/core/sock.c:1703:4: error: implicit declaration of function 'prefetch' [-Werror=implicit-function-declaration]
Caused by commit e4cbb02a1070 ("net: add a prefetch in socket backlog
processing").
I have reverted that commit for today.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] tcp: cleanup tcp_try_coalesce
From: David Miller @ 2012-05-03 5:50 UTC (permalink / raw)
To: alexander.duyck
Cc: eric.dumazet, alexander.h.duyck, netdev, edumazet,
jeffrey.t.kirsher
In-Reply-To: <4FA21A90.3010008@gmail.com>
From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Wed, 02 May 2012 22:41:36 -0700
> I think the part that has me confused is how being more precise about
> removing from truesize gets in the way of detecting abuses of
> truesize. It seems like it should be more as good as or better then
> the original approach of just using skb->len.
You can only trim from truesize if you can be absolutely certain that
you have removed any reference in the fraglist, or the page'd head, to
the entire "block" of data.
If the skb still refers to even just one byte in a particular block,
we must still charge the entire block in the truesize.
For example, NIU has three pools of power-of-2 blocks of data it
maintainers and the device pulls from to build incoming packets.
So if the chip used one of the 2048 byte buffers, we charge the entire
2048 bytes even of the packet is much smaller.
Conversely this means we cannot trim the 2048 part of the truesize of
that SKB unless we had some mechanism to know for certain 1) what the
block size of the underlying data is and 2) that we've removed all
references to that.
Currently this is not really possible, so we therefore defer truesize
adjustments.
Behaving otherwise is dangerous, because then we'd potentially end up
with a lot of memory used, but not actually accounted for by anyone.
^ permalink raw reply
* Re: [net-next PATCH v4 0/8] Managing the forwarding database(FDB)
From: Michael S. Tsirkin @ 2012-05-03 5:48 UTC (permalink / raw)
To: John Fastabend
Cc: shemminger, bhutchings, sri, hadi, jeffrey.t.kirsher, netdev,
gregory.v.rose, krkumar2, roprabhu
In-Reply-To: <4FA1ACA1.2080808@intel.com>
On Wed, May 02, 2012 at 02:52:33PM -0700, John Fastabend wrote:
> On 5/2/2012 8:08 AM, Michael S. Tsirkin wrote:
> > On Sun, Apr 15, 2012 at 01:06:37PM -0400, David Miller wrote:
> >> From: John Fastabend <john.r.fastabend@intel.com>
> >> Date: Sun, 15 Apr 2012 09:43:51 -0700
> >>
> >>> The following series is a submission for net-next to allow
> >>> embedded switches and other stacked devices other then the
> >>> Linux bridge to manage a forwarding database.
> >>>
> >>> Previously discussed here,
> >>>
> >>> http://lists.openwall.net/netdev/2012/03/19/26
> >>>
> >>> v4: propagate return codes correctly for ndo_dflt_Fdb_dump()
> >>>
> >>> v3: resolve the macvlan patch 8/8 to fix a dev_set_promiscuity()
> >>> error and add the flags field to change and get link routines.
> >>>
> >>> v2: addressed feedback from Ben Hutchings resolving a typo in the
> >>> multicast add/del routines and improving the error handling
> >>> when both NTF_SELF and NTF_MASTER are set.
> >>>
> >>> I've tested this with 'br' tool published by Stephen Hemminger
> >>> soon to be renamed 'bridge' I believe and various traffic
> >>> generators mostly pktgen, ping, and netperf.
> >>
> >> All applied, if we need any more tweaks we can just add them
> >> on top of this work.
> >>
> >> Thanks John.
> >
> > John, do you plan to update kvm userspace to use this interface?
> >
>
> No immediate plans. I would really appreciate it if you or one
> of the IBM developers working in this space took it on. Of course
> if no one steps up I guess I can eventually get at it but it will
> be sometime. For now I've been doing this manually with the bridge
> tool yet to be published.
>
> .John
It'll be easier once you publish the tool, qemu can just run
scripts like it does for ifup/ifdown now.
--
MST
^ permalink raw reply
* Re: [PATCH] sungem: Fix WakeOnLan
From: David Miller @ 2012-05-03 5:43 UTC (permalink / raw)
To: gerard.lledo; +Cc: netdev
In-Reply-To: <1335639157-6336-1-git-send-email-gerard.lledo@gmail.com>
From: Gerard Lledo <gerard.lledo@gmail.com>
Date: Sat, 28 Apr 2012 21:52:37 +0300
> From: Gerard Lledo <gerard.lledo@gmail.com>
>
> WakeOnLan was broken in this driver because gp->asleep_wol is a 1-bit
> bitfield and it was being assigned WAKE_MAGIC, which is (1 << 5).
> gp->asleep_wol remains 0 and the machine never wakes up. Fixed by casting
> gp->wake_on_lan to bool. Tested on an iBook G4.
>
> Signed-off-by: Gerard Lledo <gerard.lledo@gmail.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH v3] tilegx network driver: initial support
From: David Miller @ 2012-05-03 5:41 UTC (permalink / raw)
To: cmetcalf; +Cc: arnd, linux-kernel, netdev
In-Reply-To: <201205011759.q41HxR21020043@farm-0012.internal.tilera.com>
From: Chris Metcalf <cmetcalf@tilera.com>
Date: Mon, 17 Sep 2001 00:00:00 -0400
> +/* #define USE_SIM_PRINTF */
> +
> +#ifdef USE_SIM_PRINTF
> +
> +static __attribute__((unused, format (printf, 1, 2))) void
> +sim_printf(const char *format, ...)
...
> +/* HACK: Allow use of "sim_printf()" instead of "printk()". */
> +#define printk sim_printf
> +
> +#endif
This doesn't belong in a driver.
You want a debugging console driver that uses that special SIM output
facility instead.
Therefore, please remove this sim_printf stuff completely.
^ permalink raw reply
* Re: [PATCH 2/2] tcp: cleanup tcp_try_coalesce
From: Alexander Duyck @ 2012-05-03 5:41 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Alexander Duyck, netdev, davem, Eric Dumazet, Jeff Kirsher
In-Reply-To: <1336022373.12425.24.camel@edumazet-glaptop>
On 5/2/2012 10:19 PM, Eric Dumazet wrote:
> On Wed, 2012-05-02 at 21:58 -0700, Alexander Duyck wrote:
>> The question I have is how can you get into a case where the ksize is
>> different from the end offset plus the aligned size of skb_shared_info?
>> From what I can tell it looks like the only place we can lie is if we
>> use build_skb with the frag_size option, and in that case we are using a
>> page, not kmalloc memory. Otherwise in all other cases __alloc_skb or
>> build_skb is using ksize(skb->head) - SKB_DATA_ALIGN(struct
>> skb_shared_info) to set the end pointer, so reversing that should give
>> us the same value as ksize(skb->head).
>
> Right after skb is allocated (build_skb() or other skb_alloc...
> variants), truesize is correct by construction.
>
> Then drivers add fragments and can make truesize smaller than reality.
>
> And Intel drivers are known to abuse truesize.
>
> My last patch against iwlwifi is still waiting to make its way into
> official tree.
>
> http://www.spinics.net/lists/netdev/msg192629.html
I think the part that has me confused is how being more precise about
removing from truesize gets in the way of detecting abuses of truesize.
It seems like it should be more as good as or better then the original
approach of just using skb->len.
Then again we might just be talking in circles again. I have things
broken out into 3 patches now that are much more readable. I will email
them out in an hour or so once I do some quick tests to verify they are
building and don't break anything.
Thanks,
Alex
^ permalink raw reply
* Re: [v2 PATCH] net: Stop decapitating clones that have a head_frag
From: David Miller @ 2012-05-03 5:39 UTC (permalink / raw)
To: eric.dumazet; +Cc: alexander.h.duyck, netdev, edumazet, jeffrey.t.kirsher
In-Reply-To: <1336019638.12425.16.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 May 2012 06:33:58 +0200
> On Wed, 2012-05-02 at 21:18 -0700, Alexander Duyck wrote:
>> This change is meant ot prevent stealing the skb->head to use as a page in
>> the event that the skb->head was cloned. This allows the other clones to
>> track each other via shinfo->dataref.
>>
>> Without this we break down to two methods for tracking the reference count,
>> one being dataref, the other being the page count. As a result it becomes
>> difficult to track how many references there are to skb->head.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
...
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied, thanks.
These tests of the form:
head_frag || cloned
and
head_frag && cloned
will probably crop up in other places. Therefore we should probably
add a "skb_head_is_locked()" helper function to skbuff.h
Well, there are already two instances, which is candidate enough. :-)
^ permalink raw reply
* IPv6 bonding in VM doesn't work (was Re: bonding and IPv6 "doesn't work"?)
From: Amos Kong @ 2012-05-03 5:33 UTC (permalink / raw)
To: Chris Friesen
Cc: netdev, bridge, qemu-devel, Tomasz Chmielewski, shemminger,
David Lamparter
[-- Attachment #1: Type: text/plain, Size: 1844 bytes --]
On Wed, Jul 13, 2011 at 1:15 AM, Chris Friesen <chris.friesen@genband.com>wrote:
> On 07/12/2011 10:25 AM, Tomasz Chmielewski wrote:
>
>> On 12.07.2011 18:14, David Lamparter wrote:
>>
>
> Your bonding peer is probably looping those
>>> packets back on the other link, most likely because...
>>>
>>> Bonding Mode: load balancing (round-robin)
>>>>
>>>
>>> ... most likely because you maybe have a switch on the other side, and
>>> that switch expects you to do 802.3ad?
>>>
>>
>> It's a virtual machine, so the host shouldn't know or care much about
>> 802.3ad (I think!).
>>
>
Bonding mode 1 and 5 work when slave nics(guest) connect with same virtual
bridge in host.
other modes don't work.
>
> I suspect that connecting multiple links of a bond to the same unmanaged
> switch (or virtual bridge) is going to confuse things.
>
> Try using multiple virtual bridges instead, one for each slave in the
> bond. That way they won't interfere with each other.
>
>> Continue original thread: http://marc.info/?t=131048686200002&r=1&w=2
http://en.wikipedia.org/wiki/IEEE_802.1AX
""" With modes balance-rr, balance-xor, broadcast and 802.3ad all physical
ports in the link aggregation group _must reside on the same_ logical
switch, which in most scenarios will leave a single point of failure when
the physical switch to which both links are connected goes offline. Modes
active-backup, balance-tlb, and balance-alb can also be set up with two or
more switches."""
We could not resolve this issue by connect links to different virtual
bridges?
But it doesn't make sense to connect slave nics with same physical path,
NO stable /performance benefit.
What will happen if we connect two physical NIC directly (back-to-back)?
duplicated address detected?
BTW, openvswitch already support 802.3ad, it might resolve this issue.
Thanks, Amos
[-- Attachment #2: Type: text/html, Size: 3384 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] tcp: cleanup tcp_try_coalesce
From: David Miller @ 2012-05-03 5:25 UTC (permalink / raw)
To: eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w
Cc: alexander.duyck-Re5JQEeQqe8AvxtiuMwx3w,
alexander.h.duyck-ral2JQCrhuEAvxtiuMwx3w,
netdev-u79uwXL29TY76Z2rM5mHXA, edumazet-hpIqsD4AKlfQT0dZR+AlfA,
jeffrey.t.kirsher-ral2JQCrhuEAvxtiuMwx3w,
linville-2XuSBdqkA4R54TAoqtyWWQ,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1336022373.12425.24.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Thu, 03 May 2012 07:19:33 +0200
> My last patch against iwlwifi is still waiting to make its way into
> official tree.
>
> http://www.spinics.net/lists/netdev/msg192629.html
John, please rectify this situation.
The Intel Wireless folks said they would test it, but that was more
than a month ago.
It's not acceptable to let bug fixes rot for that long, I don't care
what their special internal testing procedure is.
If they give you further pushback, please just ignore them and apply
Eric's fix directly.
Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" 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 2/2] tcp: cleanup tcp_try_coalesce
From: Eric Dumazet @ 2012-05-03 5:19 UTC (permalink / raw)
To: Alexander Duyck
Cc: Alexander Duyck, netdev, davem, Eric Dumazet, Jeff Kirsher
In-Reply-To: <4FA21087.1080801@gmail.com>
On Wed, 2012-05-02 at 21:58 -0700, Alexander Duyck wrote:
> The question I have is how can you get into a case where the ksize is
> different from the end offset plus the aligned size of skb_shared_info?
> From what I can tell it looks like the only place we can lie is if we
> use build_skb with the frag_size option, and in that case we are using a
> page, not kmalloc memory. Otherwise in all other cases __alloc_skb or
> build_skb is using ksize(skb->head) - SKB_DATA_ALIGN(struct
> skb_shared_info) to set the end pointer, so reversing that should give
> us the same value as ksize(skb->head).
Right after skb is allocated (build_skb() or other skb_alloc...
variants), truesize is correct by construction.
Then drivers add fragments and can make truesize smaller than reality.
And Intel drivers are known to abuse truesize.
My last patch against iwlwifi is still waiting to make its way into
official tree.
http://www.spinics.net/lists/netdev/msg192629.html
^ permalink raw reply
* Re: [PATCH net v2] cdc_ether: Ignore bogus union descriptor for RNDIS devices
From: David Miller @ 2012-05-03 5:11 UTC (permalink / raw)
To: linux-201011-TzK+PxyQ8t4hFhg+JK9F0w
Cc: bjorn-yOkvZcmFvRU, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA, shaola-r/HQZD9NxM9g9hUCZPvPmw,
jrnieder-Re5JQEeQqe8AvxtiuMwx3w, oliver-GvhC2dPhHPQdnm+yROfE0A,
655387-61a8vm9lEZVf4u+23C9RwQ, stable-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <01138af4-69c3-4adb-b7f6-1e5e6f471df2-2ueSQiBKiTY7tOexoI0I+QC/G2K4zDHf@public.gmane.org>
From: Markus Kolb <linux-201011-TzK+PxyQ8t4hFhg+JK9F0w@public.gmane.org>
Date: Thu, 03 May 2012 06:57:39 +0200
> I'll build it during next rainy day and will report its success
> after some usage ;-)
Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] tcp: cleanup tcp_try_coalesce
From: Alexander Duyck @ 2012-05-03 4:58 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Alexander Duyck, netdev, davem, Eric Dumazet, Jeff Kirsher
In-Reply-To: <1336017985.12425.9.camel@edumazet-glaptop>
On 5/2/2012 9:06 PM, Eric Dumazet wrote:
> On Wed, 2012-05-02 at 20:39 -0700, Alexander Duyck wrote:
>> This change is mostly meant to help improve the readability of
>> tcp_try_coalesce. I had a few issues since there were several points when
>> the code would test for a conditional, fail, then succeed on another
>> conditional take some action, and then follow a goto back into the previous
>> conditional. I just tore all of that apart and made the whole thing one
>> linear flow with a single goto.
>>
>> Also there were multiple ways of computing the delta, the one for head_frag
>> made the least amount of sense to me since we were only dropping the
>> sk_buff so I have updated the logic for the stolen head case so that delta
>> is only truesize - sizeof(skb_buff), and for the case where we are dropping
>> the head as well it is truesize - SKB_TRUESIZE(skb_end_pointer - head).
>> This way we can also account for the head_frag with headlen == 0.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> Cc: Eric Dumazet<edumazet@google.com>
>> Cc: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>> ---
>>
> Sorry I prefer you dont touch this code like this.
>
> The truesize bits must stay as is, since it'll track drivers that lies
> about truesize.
I can understand that. But from what I can tell the only way they can
lie about truesize is to actually change the value itself. The code I
modified was just tightening things up so we didn't do things like use
the length to track the truesize like we were in the original code.
From what I can tell the new code should have been more accurate.
>> net/ipv4/tcp_input.c | 80 +++++++++++++++++++++++++++-----------------------
>> 1 files changed, 43 insertions(+), 37 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index c6f78e2..23bc3ff 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -4548,62 +4548,68 @@ static bool tcp_try_coalesce(struct sock *sk,
>> int i, delta, len = from->len;
>>
>> *fragstolen = false;
>> +
>> if (tcp_hdr(from)->fin || skb_cloned(to))
>> return false;
>> +
>> if (len<= skb_tailroom(to)) {
>> BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
>> -merge:
>> - NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
>> - TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
>> - TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
>> - return true;
>> + goto merge;
>> }
>>
>> if (skb_has_frag_list(to) || skb_has_frag_list(from))
>> return false;
>>
>> - if (skb_headlen(from) == 0&&
>> - (skb_shinfo(to)->nr_frags +
>> - skb_shinfo(from)->nr_frags<= MAX_SKB_FRAGS)) {
>> - WARN_ON_ONCE(from->head_frag);
>> - delta = from->truesize - ksize(from->head) -
>> - SKB_DATA_ALIGN(sizeof(struct sk_buff));
>
> This delta was done on purpose. We want the ksize()
The question I have is how can you get into a case where the ksize is
different from the end offset plus the aligned size of skb_shared_info?
>From what I can tell it looks like the only place we can lie is if we
use build_skb with the frag_size option, and in that case we are using a
page, not kmalloc memory. Otherwise in all other cases __alloc_skb or
build_skb is using ksize(skb->head) - SKB_DATA_ALIGN(struct
skb_shared_info) to set the end pointer, so reversing that should give
us the same value as ksize(skb->head).
>> -
>> - WARN_ON_ONCE(delta< len);
>> -copyfrags:
>> - memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
>> - skb_shinfo(from)->frags,
>> - skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
>> - skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
>> -
>> - if (skb_cloned(from))
>> - for (i = 0; i< skb_shinfo(from)->nr_frags; i++)
>> - skb_frag_ref(from, i);
>> - else
>> - skb_shinfo(from)->nr_frags = 0;
>> -
>> - to->truesize += delta;
>> - atomic_add(delta,&sk->sk_rmem_alloc);
>> - sk_mem_charge(sk, delta);
>> - to->len += len;
>> - to->data_len += len;
>> - goto merge;
>> - }
>> - if (from->head_frag&& !skb_cloned(from)) {
>> + if (skb_headlen(from) != 0) {
>> struct page *page;
>> unsigned int offset;
>>
>> - if (skb_shinfo(to)->nr_frags + skb_shinfo(from)->nr_frags>= MAX_SKB_FRAGS)
>> + if (skb_shinfo(to)->nr_frags +
>> + skb_shinfo(from)->nr_frags>= MAX_SKB_FRAGS)
>> + return false;
>> +
>> + if (!from->head_frag || skb_cloned(from))
>> return false;
>> +
>> + delta = from->truesize - sizeof(struct sk_buff);
>>
It looks like you were thinking of something here since the quote lines
were broken. This was the piece I saw in the original code that caught
my eye as probably being wrong. We are letting packets using head_frag
just report length as the truesize. This is why I favored using the end
offset. In the case of us using the head_frag we should only be
deducting SKB_DATA_ALIGN(sizeof(struct sk_buff)) (I just relized the
line above is incorrect), and in the case of us dropping the head_frag
as well we should be able to also deduct the size of the shared_info and
everything up to the offset of end.
>> +
>> page = virt_to_head_page(from->head);
>> offset = from->data - (unsigned char *)page_address(page);
>> +
>> skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
>> page, offset, skb_headlen(from));
>> *fragstolen = true;
>> - delta = len; /* we dont know real truesize... */
>> - goto copyfrags;
>> + } else {
>> + if (skb_shinfo(to)->nr_frags +
>> + skb_shinfo(from)->nr_frags> MAX_SKB_FRAGS)
>> + return false;
>> +
>> + delta = from->truesize -
>> + SKB_TRUESIZE(skb_end_pointer(from) - from->head);
> No... SKB_TRUESIZE() doesnt account of power-of-two roundings in
> kmalloc()
You used ksize in alloc_skb or build_skb to set the end pointer. All I
am doing by using the end pointer is getting the value you had already
extracted. The end pointer should be unmovable once it is set so I
wouldn't think it would be an issue to use it to compute the truesize
for the section including the sk_buff and skb->head.
>> }
>> - return false;
>> +
>> + memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
>> + skb_shinfo(from)->frags,
>> + skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
>> + skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
>> +
>> + if (!skb_cloned(from))
>> + skb_shinfo(from)->nr_frags = 0;
>> +
> You break the code here... we had an else.
Yes and no. I just realized that I didn't need the else because the for
loop below does nothing if nr_frags is 0. I suspect gcc is probably
smart enough to just skip the loop if the skb is cloned. I should
probably have added a one line comment explaining that above the loop.
>> + for (i = 0; i< skb_shinfo(from)->nr_frags; i++)
>> + skb_frag_ref(from, i);
>> +
>> + to->truesize += delta;
>> + atomic_add(delta,&sk->sk_rmem_alloc);
>> + sk_mem_charge(sk, delta);
>> + to->len += len;
>> + to->data_len += len;
>> +
>> +merge:
>> + NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
>> + TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
>> + TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
>> + return true;
>> }
>>
>> static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)
>>
> Really this patch is too hard to review.
Yeah, it is a bit difficult. After Dave pulled your patches it ended up
pushing most of the changes into this one. I'll see if I can break this
back up into smaller bits. I just needed to flush the patches I had so
I could get some feedback and stop talking in circles about the other patch.
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH net v2] cdc_ether: Ignore bogus union descriptor for RNDIS devices
From: Markus Kolb @ 2012-05-03 4:57 UTC (permalink / raw)
To: David Miller, bjorn-yOkvZcmFvRU
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA,
shaola-r/HQZD9NxM9g9hUCZPvPmw, jrnieder-Re5JQEeQqe8AvxtiuMwx3w,
oliver-GvhC2dPhHPQdnm+yROfE0A, 655387-61a8vm9lEZVf4u+23C9RwQ,
stable-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20120502.211411.560705499667456177.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
David Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org> schrieb:
>From: Bjørn Mork <bjorn-yOkvZcmFvRU@public.gmane.org>
>Date: Thu, 26 Apr 2012 14:35:10 +0200
>
>> The same comments as for v1 regarding testing applies. This is build
>> tested only. Should go through some functional testing before being
>> applied.
>
>Well? Is anyone gonna test this?
I'll build it during next rainy day and will report its success after some usage ;-)
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v2 PATCH] net: Stop decapitating clones that have a head_frag
From: Eric Dumazet @ 2012-05-03 4:33 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, Eric Dumazet, Jeff Kirsher
In-Reply-To: <20120503041842.8315.16138.stgit@gitlad.jf.intel.com>
On Wed, 2012-05-02 at 21:18 -0700, Alexander Duyck wrote:
> This change is meant ot prevent stealing the skb->head to use as a page in
> the event that the skb->head was cloned. This allows the other clones to
> track each other via shinfo->dataref.
>
> Without this we break down to two methods for tracking the reference count,
> one being dataref, the other being the page count. As a result it becomes
> difficult to track how many references there are to skb->head.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>
> net/core/skbuff.c | 9 +++++----
> net/ipv4/tcp_input.c | 9 ++-------
> 2 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 52ba2b5..9e8caa0 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1699,17 +1699,18 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
> struct splice_pipe_desc *spd, struct sock *sk)
> {
> int seg;
> - bool head_is_linear = !skb->head_frag;
> + bool head_is_locked = !skb->head_frag || skb_cloned(skb);
>
Thanks
Acked-by: Eric Dumazet <edumazet@google.com>
^ permalink raw reply
* [v2 PATCH] net: Stop decapitating clones that have a head_frag
From: Alexander Duyck @ 2012-05-03 4:18 UTC (permalink / raw)
To: netdev; +Cc: davem, Alexander Duyck, Eric Dumazet, Jeff Kirsher
This change is meant ot prevent stealing the skb->head to use as a page in
the event that the skb->head was cloned. This allows the other clones to
track each other via shinfo->dataref.
Without this we break down to two methods for tracking the reference count,
one being dataref, the other being the page count. As a result it becomes
difficult to track how many references there are to skb->head.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
net/core/skbuff.c | 9 +++++----
net/ipv4/tcp_input.c | 9 ++-------
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 52ba2b5..9e8caa0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1699,17 +1699,18 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
struct splice_pipe_desc *spd, struct sock *sk)
{
int seg;
- bool head_is_linear = !skb->head_frag;
+ bool head_is_locked = !skb->head_frag || skb_cloned(skb);
/* map the linear part :
- * If skb->head_frag is set, this 'linear' part is backed
- * by a fragment, and we can avoid a copy.
+ * If skb->head_frag is set, this 'linear' part is backed by a
+ * fragment, and if the head is not shared with any clones then
+ * we can avoid a copy since we own the head portion of this page.
*/
if (__splice_segment(virt_to_page(skb->data),
(unsigned long) skb->data & (PAGE_SIZE - 1),
skb_headlen(skb),
offset, len, skb, spd,
- head_is_linear,
+ head_is_locked,
sk, pipe))
return true;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2f696ef..c6f78e2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4589,7 +4589,7 @@ copyfrags:
to->data_len += len;
goto merge;
}
- if (from->head_frag) {
+ if (from->head_frag && !skb_cloned(from)) {
struct page *page;
unsigned int offset;
@@ -4599,12 +4599,7 @@ copyfrags:
offset = from->data - (unsigned char *)page_address(page);
skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
page, offset, skb_headlen(from));
-
- if (skb_cloned(from))
- get_page(page);
- else
- *fragstolen = true;
-
+ *fragstolen = true;
delta = len; /* we dont know real truesize... */
goto copyfrags;
}
^ permalink raw reply related
* Re: [PATCH 2/2] tcp: cleanup tcp_try_coalesce
From: Eric Dumazet @ 2012-05-03 4:06 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, Eric Dumazet, Jeff Kirsher
In-Reply-To: <20120503033901.5482.27183.stgit@gitlad.jf.intel.com>
On Wed, 2012-05-02 at 20:39 -0700, Alexander Duyck wrote:
> This change is mostly meant to help improve the readability of
> tcp_try_coalesce. I had a few issues since there were several points when
> the code would test for a conditional, fail, then succeed on another
> conditional take some action, and then follow a goto back into the previous
> conditional. I just tore all of that apart and made the whole thing one
> linear flow with a single goto.
>
> Also there were multiple ways of computing the delta, the one for head_frag
> made the least amount of sense to me since we were only dropping the
> sk_buff so I have updated the logic for the stolen head case so that delta
> is only truesize - sizeof(skb_buff), and for the case where we are dropping
> the head as well it is truesize - SKB_TRUESIZE(skb_end_pointer - head).
> This way we can also account for the head_frag with headlen == 0.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>
Sorry I prefer you dont touch this code like this.
The truesize bits must stay as is, since it'll track drivers that lies
about truesize.
> net/ipv4/tcp_input.c | 80 +++++++++++++++++++++++++++-----------------------
> 1 files changed, 43 insertions(+), 37 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index c6f78e2..23bc3ff 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4548,62 +4548,68 @@ static bool tcp_try_coalesce(struct sock *sk,
> int i, delta, len = from->len;
>
> *fragstolen = false;
> +
> if (tcp_hdr(from)->fin || skb_cloned(to))
> return false;
> +
> if (len <= skb_tailroom(to)) {
> BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
> -merge:
> - NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
> - TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
> - TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
> - return true;
> + goto merge;
> }
>
> if (skb_has_frag_list(to) || skb_has_frag_list(from))
> return false;
>
> - if (skb_headlen(from) == 0 &&
> - (skb_shinfo(to)->nr_frags +
> - skb_shinfo(from)->nr_frags <= MAX_SKB_FRAGS)) {
> - WARN_ON_ONCE(from->head_frag);
> - delta = from->truesize - ksize(from->head) -
> - SKB_DATA_ALIGN(sizeof(struct sk_buff));
This delta was done on purpose. We want the ksize()
> -
> - WARN_ON_ONCE(delta < len);
> -copyfrags:
> - memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
> - skb_shinfo(from)->frags,
> - skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
> - skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
> -
> - if (skb_cloned(from))
> - for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
> - skb_frag_ref(from, i);
> - else
> - skb_shinfo(from)->nr_frags = 0;
> -
> - to->truesize += delta;
> - atomic_add(delta, &sk->sk_rmem_alloc);
> - sk_mem_charge(sk, delta);
> - to->len += len;
> - to->data_len += len;
> - goto merge;
> - }
> - if (from->head_frag && !skb_cloned(from)) {
> + if (skb_headlen(from) != 0) {
> struct page *page;
> unsigned int offset;
>
> - if (skb_shinfo(to)->nr_frags + skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
> + if (skb_shinfo(to)->nr_frags +
> + skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
> + return false;
> +
> + if (!from->head_frag || skb_cloned(from))
> return false;
> +
> + delta = from->truesize - sizeof(struct sk_buff);
> +
> page = virt_to_head_page(from->head);
> offset = from->data - (unsigned char *)page_address(page);
> +
> skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
> page, offset, skb_headlen(from));
> *fragstolen = true;
> - delta = len; /* we dont know real truesize... */
> - goto copyfrags;
> + } else {
> + if (skb_shinfo(to)->nr_frags +
> + skb_shinfo(from)->nr_frags > MAX_SKB_FRAGS)
> + return false;
> +
> + delta = from->truesize -
> + SKB_TRUESIZE(skb_end_pointer(from) - from->head);
No... SKB_TRUESIZE() doesnt account of power-of-two roundings in
kmalloc()
> }
> - return false;
> +
> + memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
> + skb_shinfo(from)->frags,
> + skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
> + skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
> +
> + if (!skb_cloned(from))
> + skb_shinfo(from)->nr_frags = 0;
> +
You break the code here... we had an else.
> + for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
> + skb_frag_ref(from, i);
> +
> + to->truesize += delta;
> + atomic_add(delta, &sk->sk_rmem_alloc);
> + sk_mem_charge(sk, delta);
> + to->len += len;
> + to->data_len += len;
> +
> +merge:
> + NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
> + TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
> + TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
> + return true;
> }
>
> static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)
>
Really this patch is too hard to review.
Thanks
^ permalink raw reply
* Re: [PATCH 1/2] net: Stop decapitating clones that have a head_frag
From: Eric Dumazet @ 2012-05-03 3:56 UTC (permalink / raw)
To: Alexander Duyck; +Cc: netdev, davem, Eric Dumazet, Jeff Kirsher
In-Reply-To: <20120503033856.5482.70122.stgit@gitlad.jf.intel.com>
On Wed, 2012-05-02 at 20:38 -0700, Alexander Duyck wrote:
> This change is meant ot prevent stealing the skb->head to use as a page in
> the event that the skb->head was cloned. This allows the other clones to
> track each other via shinfo->dataref.
>
> Without this we break down to two methods for tracking the reference count,
> one being dataref, the other being the page count. As a result it becomes
> difficult to track how many references there are to skb->head.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>
> net/core/skbuff.c | 2 +-
> net/ipv4/tcp_input.c | 9 ++-------
> 2 files changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 52ba2b5..8703754 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1699,7 +1699,7 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
> struct splice_pipe_desc *spd, struct sock *sk)
> {
> int seg;
> - bool head_is_linear = !skb->head_frag;
> + bool head_is_linear = !skb->head_frag || skb_cloned(skb);
>
Can you chose a better name than head_is_linear maybe ?
> /* map the linear part :
> * If skb->head_frag is set, this 'linear' part is backed
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 2f696ef..c6f78e2 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4589,7 +4589,7 @@ copyfrags:
> to->data_len += len;
> goto merge;
> }
> - if (from->head_frag) {
> + if (from->head_frag && !skb_cloned(from)) {
> struct page *page;
> unsigned int offset;
>
> @@ -4599,12 +4599,7 @@ copyfrags:
> offset = from->data - (unsigned char *)page_address(page);
> skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
> page, offset, skb_headlen(from));
> -
> - if (skb_cloned(from))
> - get_page(page);
> - else
> - *fragstolen = true;
> -
> + *fragstolen = true;
> delta = len; /* we dont know real truesize... */
> goto copyfrags;
> }
Thanks
^ permalink raw reply
* [PATCH 2/2] tcp: cleanup tcp_try_coalesce
From: Alexander Duyck @ 2012-05-03 3:39 UTC (permalink / raw)
To: netdev; +Cc: davem, Alexander Duyck, Eric Dumazet, Jeff Kirsher
In-Reply-To: <20120503033018.5482.89902.stgit@gitlad.jf.intel.com>
This change is mostly meant to help improve the readability of
tcp_try_coalesce. I had a few issues since there were several points when
the code would test for a conditional, fail, then succeed on another
conditional take some action, and then follow a goto back into the previous
conditional. I just tore all of that apart and made the whole thing one
linear flow with a single goto.
Also there were multiple ways of computing the delta, the one for head_frag
made the least amount of sense to me since we were only dropping the
sk_buff so I have updated the logic for the stolen head case so that delta
is only truesize - sizeof(skb_buff), and for the case where we are dropping
the head as well it is truesize - SKB_TRUESIZE(skb_end_pointer - head).
This way we can also account for the head_frag with headlen == 0.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
net/ipv4/tcp_input.c | 80 +++++++++++++++++++++++++++-----------------------
1 files changed, 43 insertions(+), 37 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index c6f78e2..23bc3ff 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4548,62 +4548,68 @@ static bool tcp_try_coalesce(struct sock *sk,
int i, delta, len = from->len;
*fragstolen = false;
+
if (tcp_hdr(from)->fin || skb_cloned(to))
return false;
+
if (len <= skb_tailroom(to)) {
BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));
-merge:
- NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
- TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
- TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
- return true;
+ goto merge;
}
if (skb_has_frag_list(to) || skb_has_frag_list(from))
return false;
- if (skb_headlen(from) == 0 &&
- (skb_shinfo(to)->nr_frags +
- skb_shinfo(from)->nr_frags <= MAX_SKB_FRAGS)) {
- WARN_ON_ONCE(from->head_frag);
- delta = from->truesize - ksize(from->head) -
- SKB_DATA_ALIGN(sizeof(struct sk_buff));
-
- WARN_ON_ONCE(delta < len);
-copyfrags:
- memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
- skb_shinfo(from)->frags,
- skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
- skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
-
- if (skb_cloned(from))
- for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
- skb_frag_ref(from, i);
- else
- skb_shinfo(from)->nr_frags = 0;
-
- to->truesize += delta;
- atomic_add(delta, &sk->sk_rmem_alloc);
- sk_mem_charge(sk, delta);
- to->len += len;
- to->data_len += len;
- goto merge;
- }
- if (from->head_frag && !skb_cloned(from)) {
+ if (skb_headlen(from) != 0) {
struct page *page;
unsigned int offset;
- if (skb_shinfo(to)->nr_frags + skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
+ if (skb_shinfo(to)->nr_frags +
+ skb_shinfo(from)->nr_frags >= MAX_SKB_FRAGS)
+ return false;
+
+ if (!from->head_frag || skb_cloned(from))
return false;
+
+ delta = from->truesize - sizeof(struct sk_buff);
+
page = virt_to_head_page(from->head);
offset = from->data - (unsigned char *)page_address(page);
+
skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
page, offset, skb_headlen(from));
*fragstolen = true;
- delta = len; /* we dont know real truesize... */
- goto copyfrags;
+ } else {
+ if (skb_shinfo(to)->nr_frags +
+ skb_shinfo(from)->nr_frags > MAX_SKB_FRAGS)
+ return false;
+
+ delta = from->truesize -
+ SKB_TRUESIZE(skb_end_pointer(from) - from->head);
}
- return false;
+
+ memcpy(skb_shinfo(to)->frags + skb_shinfo(to)->nr_frags,
+ skb_shinfo(from)->frags,
+ skb_shinfo(from)->nr_frags * sizeof(skb_frag_t));
+ skb_shinfo(to)->nr_frags += skb_shinfo(from)->nr_frags;
+
+ if (!skb_cloned(from))
+ skb_shinfo(from)->nr_frags = 0;
+
+ for (i = 0; i < skb_shinfo(from)->nr_frags; i++)
+ skb_frag_ref(from, i);
+
+ to->truesize += delta;
+ atomic_add(delta, &sk->sk_rmem_alloc);
+ sk_mem_charge(sk, delta);
+ to->len += len;
+ to->data_len += len;
+
+merge:
+ NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_TCPRCVCOALESCE);
+ TCP_SKB_CB(to)->end_seq = TCP_SKB_CB(from)->end_seq;
+ TCP_SKB_CB(to)->ack_seq = TCP_SKB_CB(from)->ack_seq;
+ return true;
}
static void kfree_skb_partial(struct sk_buff *skb, bool head_stolen)
^ permalink raw reply related
* [PATCH 1/2] net: Stop decapitating clones that have a head_frag
From: Alexander Duyck @ 2012-05-03 3:38 UTC (permalink / raw)
To: netdev; +Cc: davem, Alexander Duyck, Eric Dumazet, Jeff Kirsher
In-Reply-To: <20120503033018.5482.89902.stgit@gitlad.jf.intel.com>
This change is meant ot prevent stealing the skb->head to use as a page in
the event that the skb->head was cloned. This allows the other clones to
track each other via shinfo->dataref.
Without this we break down to two methods for tracking the reference count,
one being dataref, the other being the page count. As a result it becomes
difficult to track how many references there are to skb->head.
Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
net/core/skbuff.c | 2 +-
net/ipv4/tcp_input.c | 9 ++-------
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 52ba2b5..8703754 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1699,7 +1699,7 @@ static bool __skb_splice_bits(struct sk_buff *skb, struct pipe_inode_info *pipe,
struct splice_pipe_desc *spd, struct sock *sk)
{
int seg;
- bool head_is_linear = !skb->head_frag;
+ bool head_is_linear = !skb->head_frag || skb_cloned(skb);
/* map the linear part :
* If skb->head_frag is set, this 'linear' part is backed
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2f696ef..c6f78e2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4589,7 +4589,7 @@ copyfrags:
to->data_len += len;
goto merge;
}
- if (from->head_frag) {
+ if (from->head_frag && !skb_cloned(from)) {
struct page *page;
unsigned int offset;
@@ -4599,12 +4599,7 @@ copyfrags:
offset = from->data - (unsigned char *)page_address(page);
skb_fill_page_desc(to, skb_shinfo(to)->nr_frags,
page, offset, skb_headlen(from));
-
- if (skb_cloned(from))
- get_page(page);
- else
- *fragstolen = true;
-
+ *fragstolen = true;
delta = len; /* we dont know real truesize... */
goto copyfrags;
}
^ permalink raw reply related
* [PATCH 0/2] Cleanups for head_frag usage and tcp_try_coalese
From: Alexander Duyck @ 2012-05-03 3:38 UTC (permalink / raw)
To: netdev; +Cc: davem
I wrote up these patches earlier today to address issues related to the
splitting of reference counting into two seperate counts in the case of
skbs containing a head frag. In addition I have a secondary patch for
tcp_try_coalesce which is meant to be a general cleanup as I found it
difficult to read through the code as it was.
I have done some performance testing on the series with netperf and saw no
appreciable difference after these changes for the non-cloned case.
---
Alexander Duyck (2):
tcp: cleanup tcp_try_coalesce
net: Stop decapitating clones that have a head_frag
net/core/skbuff.c | 2 +
net/ipv4/tcp_input.c | 85 +++++++++++++++++++++++++-------------------------
2 files changed, 44 insertions(+), 43 deletions(-)
--
^ 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