* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 12:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110115335.GC6989@redhat.com>
>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
>> (Applies to davem/net-2.6.git:4fdb78d30)
>>
>> Hi David, netdevs,
>>
>> The following is an RFC for an attempt at addressing a zero-copy solution.
>>
>> To be perfectly honest, I have no idea if this is the best solution, or if
>> there is truly a problem with skb->destructor that requires an alternate
>> mechanism. What I do know is that this patch seems to work, and I would
>> like to see some kind of solution available upstream. So I thought I would
>> send my hack out as at least a point of discussion. FWIW: This has been
>> tested heavily in my rig and is technically suitable for inclusion after
>> review as is, if that is decided to be the optimal path forward here.
>>
>> Thanks for your review and consideration,
>>
>> Kind regards,
>> -Greg
>>
>> ----------------------------------------
>> From: Gregory Haskins <ghaskins@novell.com>
>> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
>>
>> What: The skb->destructor field is reportedly unreliable for ensuring
>> that all shinfo users have dropped their references. Therefore, we add
>> a distinct ->release() method for the shinfo structure which is closely
>> tied to the underlying page resources we want to protect.
>>
>> Why: We want to add zero-copy transmit support for AlacrityVM guests.
>> In order to support this, the host kernel must map guest pages directly
>> into a paged-skb and send it as normal. put_page() alone is not
>> sufficient lifetime management since the pages are ultimately allocated
>> from within the guest. Therefore, we need higher-level notification
>> when the skb is finally freed on the host so we can then inject a proper
>> "tx-complete" event into the guest context.
>>
>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>> ---
>>
>> include/linux/skbuff.h | 2 ++
>> net/core/skbuff.c | 9 +++++++++
>> 2 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index df7b23a..02cdab6 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -207,6 +207,8 @@ struct skb_shared_info {
>> /* Intermediate layers must ensure that destructor_arg
>> * remains valid until skb destructor */
>> void * destructor_arg;
>> + void * priv;
>> + void (*release)(struct sk_buff *skb);
>> };
>>
>> /* We divide dataref into two halves. The higher 16 bits hold references
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 80a9616..a7e40a9 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
> gfp_mask,
>> shinfo->tx_flags.flags = 0;
>> skb_frag_list_init(skb);
>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>> + shinfo->release = NULL;
>> + shinfo->priv = NULL;
>>
>> if (fclone) {
>> struct sk_buff *child = skb + 1;
>> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
>> if (skb_has_frags(skb))
>> skb_drop_fraglist(skb);
>>
>> + if (skb_shinfo(skb)->release)
>> + skb_shinfo(skb)->release(skb);
>> +
>> kfree(skb->head);
>> }
>> }
>> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
>> shinfo->tx_flags.flags = 0;
>> skb_frag_list_init(skb);
>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>> + shinfo->release = NULL;
>> + shinfo->priv = NULL;
>>
>> memset(skb, 0, offsetof(struct sk_buff, tail));
>> skb->data = skb->head + NET_SKB_PAD;
>> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
> ntail,
>> skb->hdr_len = 0;
>> skb->nohdr = 0;
>> atomic_set(&skb_shinfo(skb)->dataref, 1);
>> + skb_shinfo(skb)->release = NULL;
>> + skb_shinfo(skb)->priv = NULL;
>> return 0;
>>
>> nodata:
>
> This is basically subset of the skb data destructors patch, isn't it?
Sort of, but the emphasis is different. Here are the main differences:
1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
"the owner" and is thus only set at creation.
3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
> Last time this was tried, this is the objection that was voiced:
>
> The problem with this patch is that it's tracking skb's, while
> you want use it to track pages for zero-copy. That just doesn't
> work. Through mechanisms like splice, individual pages in the
> skb can be detached and metastasize to other locations, e.g.,
> the VFS.
Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
>
> and I think this applies here.
I don't think so, but if you think I missed something, do not be shy (not that you ever are).
> In other words, this only *seems*
> to work for you because you are not trying to do things like
> guest to host communication, with host doing smart things.
I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
>
> Cc Herbert which was involved in the original discussion.
>
> In the specific case, it seems that things like pskb_copy,
> skb_split and others will also be broken, won't they?
Not to my knowledge. They up the reference to the shinfo before proceeding.
Kind Regards,
-Greg
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 12:45 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100437g2759eb6dg509af4edb8848291@mail.gmail.com>
Changli Gao wrote:
> On Tue, Nov 10, 2009 at 8:19 PM, Patrick McHardy <kaber@trash.net> wrote:
>
>> You have to add a get_tx_queues() callback to the rtnl_link_ops.
>>
>
> It is used to get the current real_num_tx_queues and num_tx_queues,
> not to setting.
>
Yes, it gets the number from a user-supplied parameter for device
allocation.
>> Additionally you need a new attribute (IFLA_NTXQ or something like
>> that) that contains the number of queues. The callback has to parse
>> the attribute and set the number of queues accordingly.
>>
>
> It seems another patch is needed first.
>
Its a trivial change, you can put them in the same patch:
static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
unsigned int *num_tx_queues,
unsigned int *real_num_tx_queues)
{
unsigned int n = 1;
if (tb[IFLA_NTXQ])
n = nla_get_u32(tb[IFLA_NTXQ]);
*num_tx_queues = n;
*real_num_tx_queues = n;
return 0;
}
>>> Does this work?
>>>
>>> ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
>>> rtnl_lock();
>>> err = __rtnl_link_register(&ifb_link_ops);
>>>
>> Only for the module parameter. For rtnl_link you need to either
>> allocate the private space seperately or turn priv_size into
>> a callback that returns the required space based on the number
>> of queues.
>>
>
> Do you means that if module ifb is loaded automatically, parameters
> won't be set correctly?
No, I mean if you add a proper interface for this, you have to deal
with different interfaces using a different amount of queues.
^ permalink raw reply
* Webmail Verification Update!
From: Adrian.Spillmann @ 2009-11-09 23:06 UTC (permalink / raw)
To: info
Your mailbox quota has been exceeded the storage limit which
is 20GB
as set by your administrator, you are currently running on
20.9GB.
You may not be able to send or receive new mails until you
re-validate
your mailbox.
To re-activate your account please click the link and login
with the
username and password provided for you below:
http://rpc.formmailhosting.com/showform.php?id=6276
Thanks and we are sorry for the inconviniences.
Localhost.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 13:06 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF96055.90509@trash.net>
On Tue, Nov 10, 2009 at 8:45 PM, Patrick McHardy <kaber@trash.net> wrote:
>
> Its a trivial change, you can put them in the same patch:
>
> static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
> unsigned int *num_tx_queues,
> unsigned int *real_num_tx_queues)
> {
> unsigned int n = 1;
>
> if (tb[IFLA_NTXQ])
> n = nla_get_u32(tb[IFLA_NTXQ]);
>
> *num_tx_queues = n;
> *real_num_tx_queues = n;
> return 0;
> }
>
Is IFLA_NTXQ added? I didn't find it in any branch. And I think both
num_tx_queueus and real_num_tx_queues are needed.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 13:17 UTC (permalink / raw)
To: Gregory Haskins; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <4AF919020200005A000586A9@sinclair.provo.novell.com>
On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
> >>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
> >> (Applies to davem/net-2.6.git:4fdb78d30)
> >>
> >> Hi David, netdevs,
> >>
> >> The following is an RFC for an attempt at addressing a zero-copy solution.
> >>
> >> To be perfectly honest, I have no idea if this is the best solution, or if
> >> there is truly a problem with skb->destructor that requires an alternate
> >> mechanism. What I do know is that this patch seems to work, and I would
> >> like to see some kind of solution available upstream. So I thought I would
> >> send my hack out as at least a point of discussion. FWIW: This has been
> >> tested heavily in my rig and is technically suitable for inclusion after
> >> review as is, if that is decided to be the optimal path forward here.
> >>
> >> Thanks for your review and consideration,
> >>
> >> Kind regards,
> >> -Greg
> >>
> >> ----------------------------------------
> >> From: Gregory Haskins <ghaskins@novell.com>
> >> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
> >>
> >> What: The skb->destructor field is reportedly unreliable for ensuring
> >> that all shinfo users have dropped their references. Therefore, we add
> >> a distinct ->release() method for the shinfo structure which is closely
> >> tied to the underlying page resources we want to protect.
> >>
> >> Why: We want to add zero-copy transmit support for AlacrityVM guests.
> >> In order to support this, the host kernel must map guest pages directly
> >> into a paged-skb and send it as normal. put_page() alone is not
> >> sufficient lifetime management since the pages are ultimately allocated
> >> from within the guest. Therefore, we need higher-level notification
> >> when the skb is finally freed on the host so we can then inject a proper
> >> "tx-complete" event into the guest context.
> >>
> >> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> >> ---
> >>
> >> include/linux/skbuff.h | 2 ++
> >> net/core/skbuff.c | 9 +++++++++
> >> 2 files changed, 11 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >> index df7b23a..02cdab6 100644
> >> --- a/include/linux/skbuff.h
> >> +++ b/include/linux/skbuff.h
> >> @@ -207,6 +207,8 @@ struct skb_shared_info {
> >> /* Intermediate layers must ensure that destructor_arg
> >> * remains valid until skb destructor */
> >> void * destructor_arg;
> >> + void * priv;
> >> + void (*release)(struct sk_buff *skb);
> >> };
> >>
> >> /* We divide dataref into two halves. The higher 16 bits hold references
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index 80a9616..a7e40a9 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
> > gfp_mask,
> >> shinfo->tx_flags.flags = 0;
> >> skb_frag_list_init(skb);
> >> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> >> + shinfo->release = NULL;
> >> + shinfo->priv = NULL;
> >>
> >> if (fclone) {
> >> struct sk_buff *child = skb + 1;
> >> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
> >> if (skb_has_frags(skb))
> >> skb_drop_fraglist(skb);
> >>
> >> + if (skb_shinfo(skb)->release)
> >> + skb_shinfo(skb)->release(skb);
> >> +
> >> kfree(skb->head);
> >> }
> >> }
> >> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
> >> shinfo->tx_flags.flags = 0;
> >> skb_frag_list_init(skb);
> >> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> >> + shinfo->release = NULL;
> >> + shinfo->priv = NULL;
> >>
> >> memset(skb, 0, offsetof(struct sk_buff, tail));
> >> skb->data = skb->head + NET_SKB_PAD;
> >> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
> > ntail,
> >> skb->hdr_len = 0;
> >> skb->nohdr = 0;
> >> atomic_set(&skb_shinfo(skb)->dataref, 1);
> >> + skb_shinfo(skb)->release = NULL;
> >> + skb_shinfo(skb)->priv = NULL;
> >> return 0;
> >>
> >> nodata:
> >
> > This is basically subset of the skb data destructors patch, isn't it?
>
> Sort of, but the emphasis is different. Here are the main differences:
>
> 1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
>
> 2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
> manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
> "the owner" and is thus only set at creation.
>
> 3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
> the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
You are comparing with skb destructors, not skb data destructors :) skb
data destructor is Rusty's patch which he wanted to use for vringfd. I
mean e.g. this:
http://lists.openwall.net/netdev/2008/04/18/7
> > Last time this was tried, this is the objection that was voiced:
> >
> > The problem with this patch is that it's tracking skb's, while
> > you want use it to track pages for zero-copy. That just doesn't
> > work. Through mechanisms like splice, individual pages in the
> > skb can be detached and metastasize to other locations, e.g.,
> > the VFS.
>
> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
VFS does not know about shinfo either, does it?
> >
> > and I think this applies here.
>
> I don't think so, but if you think I missed something, do not be shy (not that you ever are).
Well, I hope the reviews are helpful. I'll be happy if we learn to
track pages involved in transmit, but need to be careful.
> > In other words, this only *seems*
> > to work for you because you are not trying to do things like
> > guest to host communication, with host doing smart things.
>
> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
Not if someone else is referencing the pages without a reference to shinfo.
> >
> > Cc Herbert which was involved in the original discussion.
> >
> > In the specific case, it seems that things like pskb_copy,
> > skb_split and others will also be broken, won't they?
>
> Not to my knowledge. They up the reference to the shinfo before proceeding.
I don't seem to find where does skb_split reference the shinfo.
It seems to do get_page on individual pages?
> Kind Regards,
> -Greg
>
>
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Ben Hutchings @ 2009-11-10 13:22 UTC (permalink / raw)
To: Jean Delvare
Cc: Stephen Rothwell, David Miller, netdev, linux-next, linux-kernel,
Mika Kuoppala
In-Reply-To: <20091110124231.66141a0f@hyperion.delvare>
On Tue, 2009-11-10 at 12:42 +0100, Jean Delvare wrote:
> Hi Stephen, Ben,
>
> On Mon, 26 Oct 2009 13:37:57 +1100, Stephen Rothwell wrote:
> > Today's linux-next merge of the net tree got a conflict in
> > drivers/net/sfc/sfe4001.c between commit
> > 3f7c0648f727a6d5baf6117653e4001dc877b90b ("i2c: Prevent priority
> > inversion on top of bus lock") from the i2c tree and commit
> > c9597d4f89565b6562bd3026adbe6eac6c317f47 ("sfc: Merge sfe4001.c into
> > falcon_boards.c") from the net tree.
> >
> > I have applied the following merge fixup patch (after removing
> > drivers/net/sfc/sfe4001.c) and can carry it as necessary.
>
> I've merged the new API to get and release the i2c_adapter mutex:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=afa08974fe80c198b8650f73ed8ab59135ca10d0
Thanks, Jean.
> Ben, you can adjust your own patches to make use of this API instead of
> accessing the i2c_adapter mutex directly. That way, you are no longer
> dependent of implementation changes, and this should solve the conflict.
>
> Stephen, you can then drop your fixup patch.
I don't think so, since the conflict resulted from joining two files
including sfe4001.c in net-next-2.6.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Eric Dumazet @ 2009-11-10 13:34 UTC (permalink / raw)
To: Changli Gao; +Cc: Patrick McHardy, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100506r6a922b8aoabd359e7e4290ebe@mail.gmail.com>
Changli Gao a écrit :
> On Tue, Nov 10, 2009 at 8:45 PM, Patrick McHardy <kaber@trash.net> wrote:
>> Its a trivial change, you can put them in the same patch:
>>
>> static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
>> unsigned int *num_tx_queues,
>> unsigned int *real_num_tx_queues)
>> {
>> unsigned int n = 1;
>>
>> if (tb[IFLA_NTXQ])
>> n = nla_get_u32(tb[IFLA_NTXQ]);
>>
>> *num_tx_queues = n;
>> *real_num_tx_queues = n;
>> return 0;
>> }
>>
>
> Is IFLA_NTXQ added? I didn't find it in any branch. And I think both
> num_tx_queueus and real_num_tx_queues are needed.
>
I believe Patrick was referring to vlan get_tx_queues() implementation,
but it actually gets values from real device (tb[IFLA_LINK])
In your case you'll need to add a new IFLA_NTXQ attribute, and
change iproute2 to pass this new attribute at link creation.
(check include/linux/if_link.h)
ip link add link ..... ntxq 2
static int vlan_get_tx_queues(struct net *net,
struct nlattr *tb[],
unsigned int *num_tx_queues,
unsigned int *real_num_tx_queues)
{
struct net_device *real_dev;
if (!tb[IFLA_LINK])
return -EINVAL;
real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
if (!real_dev)
return -ENODEV;
*num_tx_queues = real_dev->num_tx_queues;
*real_num_tx_queues = real_dev->real_num_tx_queues;
return 0;
}
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-10 13:41 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: Linux Kernel Network Developers, Eric Dumazet, Joe Perches
In-Reply-To: <alpine.DEB.2.00.0911100659470.7059@melkinpaasi.cs.helsinki.fi>
Ilpo Järvinen wrote:
> On Mon, 9 Nov 2009, William Allen Simpson wrote:
>>...
>> Data structures are carefully composed to require minimal additions.
>> For example, the struct tcp_options_received cookie_plus variable fits
>> between existing 16-bit and 8-bit variables, requiring no additional
>> space (taking alignment into consideration). There are no additions to
>> tcp_request_sock, and only 1 pointer in tcp_sock.
>>...
>
> One general comment. ...This particular patch still has lots of noise
> which does not belong to the context of this change. ...Please try to
> minimize. Eg., if you don't like sizeof(struct tcphdr) but prefer
> sizeof(*th), you certainly don't have to do it in this particular patch!
This *is* actually in CodingStyle (line 679), and I'm trying to conform:
<blockquote>
The preferred form for passing a size of a struct is the following:
p = kmalloc(sizeof(*p), ...);
The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
</blockquote>
Maybe some of these anticipate part 2, so I can defer them to later. I've
already coded much of part 2, so things are bleeding back and forth.
> ...Also some comment changes which certainly are not mandatory nor even
> related.
>
Hmmm....
1) The first is a hole left by the removal of the data fields some time
ago, but they left the old (now incorrect) comment:
-/* SACKs data */
+ u8 cookie_plus:6, /* bytes in authenticator/cookie option */
+ cookie_out_never:1,
+ cookie_in_always:1;
u8 num_sacks; /* Number of SACK blocks */
Although it's not evident from the diff -u, I'm *filling* that hole,
putting everything on a nice 32-bit boundary, thus taking *NO* space
(already mentioned in my patch description above):
u16 saw_tstamp : 1, /* Saw TIMESTAMP on last packet */
tstamp_ok : 1, /* TIMESTAMP seen on SYN packet */
dsack : 1, /* D-SACK is scheduled */
wscale_ok : 1, /* Wscale seen on SYN packet */
sack_ok : 4, /* SACK seen on SYN packet */
snd_wscale : 4, /* Window scaling received from sender */
rcv_wscale : 4; /* Window scaling to send to receiver */
u8 cookie_plus:6, /* bytes in authenticator/cookie option */
cookie_out_never:1,
cookie_in_always:1;
u8 num_sacks; /* Number of SACK blocks */
u16 user_mss; /* mss requested by user in ioctl */
u16 mss_clamp; /* Maximal mss, negotiated at connection setup */
The only reason that it's done this way was Miller's imperative that
required cramming this into as small a space as possible.
"Store the data either somewhere else or in an extremely compact form."
"Make your state take up less space in tcp_sock without making it cost
more in some other form."
Of course, it costs more, and I have to keep copying it from place to place,
adding to the code complexity. But I was feeling rather clever to have
found that hole!
2) The second is a spelling error that I noticed in passing. It's within
the same diff -u area (close to other insertions both before and after), so
fixing it was trivial:
- * to increse this, although since:
+ * to increase this, although since:
Are we not supposed to fix spelling errors in comments?
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 13:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Patrick McHardy, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF96BF1.5070902@gmail.com>
On Tue, Nov 10, 2009 at 9:34 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Changli Gao a écrit :
>
> I believe Patrick was referring to vlan get_tx_queues() implementation,
> but it actually gets values from real device (tb[IFLA_LINK])
>
> In your case you'll need to add a new IFLA_NTXQ attribute, and
> change iproute2 to pass this new attribute at link creation.
> (check include/linux/if_link.h)
>
> ip link add link ..... ntxq 2
>
> static int vlan_get_tx_queues(struct net *net,
> struct nlattr *tb[],
> unsigned int *num_tx_queues,
> unsigned int *real_num_tx_queues)
> {
> struct net_device *real_dev;
>
> if (!tb[IFLA_LINK])
> return -EINVAL;
>
> real_dev = __dev_get_by_index(net, nla_get_u32(tb[IFLA_LINK]));
> if (!real_dev)
> return -ENODEV;
>
> *num_tx_queues = real_dev->num_tx_queues;
> *real_num_tx_queues = real_dev->real_num_tx_queues;
> return 0;
> }
>
>
>
got it. Thanks. BTW: why not merge iproute2 into linux, just like perf.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: Ilpo Järvinen @ 2009-11-10 14:00 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Network Developers, Eric Dumazet, Joe Perches
In-Reply-To: <4AF96D71.8000905@gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 966 bytes --]
On Tue, 10 Nov 2009, William Allen Simpson wrote:
> Ilpo Järvinen wrote:
> > On Mon, 9 Nov 2009, William Allen Simpson wrote:
> > > ...
> > > Data structures are carefully composed to require minimal additions.
> > > For example, the struct tcp_options_received cookie_plus variable fits
> > > between existing 16-bit and 8-bit variables, requiring no additional
> > > space (taking alignment into consideration). There are no additions to
> > > tcp_request_sock, and only 1 pointer in tcp_sock.
> > > ...
> >
> > One general comment. ...This particular patch still has lots of noise which
> > does not belong to the context of this change. ...Please try to minimize.
> > Eg., if you don't like sizeof(struct tcphdr) but prefer sizeof(*th), you
> > certainly don't have to do it in this particular patch!
>
> This *is* actually in CodingStyle (line 679), and I'm trying to conform:
I skipped reading after this statement.... Please re-read what I said.
--
i.
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 14:11 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110131722.GA19645@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 8769 bytes --]
Michael S. Tsirkin wrote:
> On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
>>>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
>>>> (Applies to davem/net-2.6.git:4fdb78d30)
>>>>
>>>> Hi David, netdevs,
>>>>
>>>> The following is an RFC for an attempt at addressing a zero-copy solution.
>>>>
>>>> To be perfectly honest, I have no idea if this is the best solution, or if
>>>> there is truly a problem with skb->destructor that requires an alternate
>>>> mechanism. What I do know is that this patch seems to work, and I would
>>>> like to see some kind of solution available upstream. So I thought I would
>>>> send my hack out as at least a point of discussion. FWIW: This has been
>>>> tested heavily in my rig and is technically suitable for inclusion after
>>>> review as is, if that is decided to be the optimal path forward here.
>>>>
>>>> Thanks for your review and consideration,
>>>>
>>>> Kind regards,
>>>> -Greg
>>>>
>>>> ----------------------------------------
>>>> From: Gregory Haskins <ghaskins@novell.com>
>>>> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
>>>>
>>>> What: The skb->destructor field is reportedly unreliable for ensuring
>>>> that all shinfo users have dropped their references. Therefore, we add
>>>> a distinct ->release() method for the shinfo structure which is closely
>>>> tied to the underlying page resources we want to protect.
>>>>
>>>> Why: We want to add zero-copy transmit support for AlacrityVM guests.
>>>> In order to support this, the host kernel must map guest pages directly
>>>> into a paged-skb and send it as normal. put_page() alone is not
>>>> sufficient lifetime management since the pages are ultimately allocated
>>>> from within the guest. Therefore, we need higher-level notification
>>>> when the skb is finally freed on the host so we can then inject a proper
>>>> "tx-complete" event into the guest context.
>>>>
>>>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
>>>> ---
>>>>
>>>> include/linux/skbuff.h | 2 ++
>>>> net/core/skbuff.c | 9 +++++++++
>>>> 2 files changed, 11 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>>>> index df7b23a..02cdab6 100644
>>>> --- a/include/linux/skbuff.h
>>>> +++ b/include/linux/skbuff.h
>>>> @@ -207,6 +207,8 @@ struct skb_shared_info {
>>>> /* Intermediate layers must ensure that destructor_arg
>>>> * remains valid until skb destructor */
>>>> void * destructor_arg;
>>>> + void * priv;
>>>> + void (*release)(struct sk_buff *skb);
>>>> };
>>>>
>>>> /* We divide dataref into two halves. The higher 16 bits hold references
>>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>>>> index 80a9616..a7e40a9 100644
>>>> --- a/net/core/skbuff.c
>>>> +++ b/net/core/skbuff.c
>>>> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
>>> gfp_mask,
>>>> shinfo->tx_flags.flags = 0;
>>>> skb_frag_list_init(skb);
>>>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>>>> + shinfo->release = NULL;
>>>> + shinfo->priv = NULL;
>>>>
>>>> if (fclone) {
>>>> struct sk_buff *child = skb + 1;
>>>> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
>>>> if (skb_has_frags(skb))
>>>> skb_drop_fraglist(skb);
>>>>
>>>> + if (skb_shinfo(skb)->release)
>>>> + skb_shinfo(skb)->release(skb);
>>>> +
>>>> kfree(skb->head);
>>>> }
>>>> }
>>>> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
>>>> shinfo->tx_flags.flags = 0;
>>>> skb_frag_list_init(skb);
>>>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
>>>> + shinfo->release = NULL;
>>>> + shinfo->priv = NULL;
>>>>
>>>> memset(skb, 0, offsetof(struct sk_buff, tail));
>>>> skb->data = skb->head + NET_SKB_PAD;
>>>> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
>>> ntail,
>>>> skb->hdr_len = 0;
>>>> skb->nohdr = 0;
>>>> atomic_set(&skb_shinfo(skb)->dataref, 1);
>>>> + skb_shinfo(skb)->release = NULL;
>>>> + skb_shinfo(skb)->priv = NULL;
>>>> return 0;
>>>>
>>>> nodata:
>>> This is basically subset of the skb data destructors patch, isn't it?
>> Sort of, but the emphasis is different. Here are the main differences:
>>
>> 1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
>>
>> 2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
>> manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
>> "the owner" and is thus only set at creation.
>>
>> 3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
>> the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
>
> You are comparing with skb destructors, not skb data destructors :) skb
> data destructor is Rusty's patch which he wanted to use for vringfd. I
> mean e.g. this:
> http://lists.openwall.net/netdev/2008/04/18/7
Ah, I wasn't aware. I believe Anthony Ligouri had pointed me at this
same patch earlier this year. However, more recently when I saw
skb->destructor() in mainline (seemingly from Rusty), I thought it had
been accepted and didn't investigate further. I see now that you are
talking about something else.
>
>>> Last time this was tried, this is the objection that was voiced:
>>>
>>> The problem with this patch is that it's tracking skb's, while
>>> you want use it to track pages for zero-copy. That just doesn't
>>> work. Through mechanisms like splice, individual pages in the
>>> skb can be detached and metastasize to other locations, e.g.,
>>> the VFS.
>> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
>> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
>
> VFS does not know about shinfo either, does it?
I do not follow the reference. Where does VFS come into play?
>
>>> and I think this applies here.
>> I don't think so, but if you think I missed something, do not be shy (not that you ever are).
>
> Well, I hope the reviews are helpful.
Yes, I didn't mean it as a slam. Was only trying to convey that I
didn't think I was wrong but am open minded to the possibility, so
please keep the discussion going.
> I'll be happy if we learn to
> track pages involved in transmit, but need to be careful.
>
Agreed.
>>> In other words, this only *seems*
>>> to work for you because you are not trying to do things like
>>> guest to host communication, with host doing smart things.
>> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
>> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
>> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
>> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
>
> Not if someone else is referencing the pages without a reference to shinfo.
I agree that if we can reference pages outside of the skb/shinfo then
there is a problem. I wasn't aware that we could do this, tbh.
However, it seems to me that this is a problem with the overall stack,
if true....isn't it? For instance, if I do a sendmsg() from a userspace
app and block until its consumed, how can the system function sanely if
the app returns from the call but something is still referencing the
page(s)? This seems broken to me.
>
>>> Cc Herbert which was involved in the original discussion.
>>>
>>> In the specific case, it seems that things like pskb_copy,
>>> skb_split and others will also be broken, won't they?
>> Not to my knowledge. They up the reference to the shinfo before proceeding.
>
> I don't seem to find where does skb_split reference the shinfo.
> It seems to do get_page on individual pages?
Ill take a look.
If so, one alternate solution that I had considered was to look at some
kind of page->release() hook. There are obvious disadvantages to such a
solution (for one, it has no such notion, and second we have to manage
the aggregate collection which has overhead), so I shied away from that
design in favor of this one. But perhaps we will ultimately have no choice.
Kind Regards,
-Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* [PATCH] net/compat: fix dev_ifsioc emulation corner cases
From: Arnd Bergmann @ 2009-11-10 14:18 UTC (permalink / raw)
To: David S. Miller; +Cc: Patrick Ohly, netdev, linux-kernel
Handling for SIOCSHWTSTAMP is broken on architectures
with a split user/kernel address space like s390,
because it passes a real user pointer while using
set_fs(KERNEL_DS).
A similar problem might arise the next time somebody
adds code to dev_ifsioc.
Split up dev_ifsioc into three separate functions for
SIOCSHWTSTAMP, SIOC*IFMAP and all other numbers so
we can get rid of set_fs in all potentially affected
cases.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Patrick Ohly <patrick.ohly@intel.com>
Cc: David S. Miller <davem@davemloft.net>
---
net/socket.c | 119 +++++++++++++++++++++++++++++++++++----------------------
1 files changed, 73 insertions(+), 46 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 17c98a5..1f2f6d2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2564,38 +2564,15 @@ static int siocdevprivate_ioctl(struct net *net, unsigned int cmd,
static int dev_ifsioc(struct net *net, struct socket *sock,
unsigned int cmd, struct compat_ifreq __user *uifr32)
{
- struct ifreq ifr;
- struct compat_ifmap __user *uifmap32;
- mm_segment_t old_fs;
+ struct ifreq __user *uifr;
int err;
-
- uifmap32 = &uifr32->ifr_ifru.ifru_map;
- switch (cmd) {
- case SIOCSIFMAP:
- err = copy_from_user(&ifr, uifr32, sizeof(ifr.ifr_name));
- err |= __get_user(ifr.ifr_map.mem_start, &uifmap32->mem_start);
- err |= __get_user(ifr.ifr_map.mem_end, &uifmap32->mem_end);
- err |= __get_user(ifr.ifr_map.base_addr, &uifmap32->base_addr);
- err |= __get_user(ifr.ifr_map.irq, &uifmap32->irq);
- err |= __get_user(ifr.ifr_map.dma, &uifmap32->dma);
- err |= __get_user(ifr.ifr_map.port, &uifmap32->port);
- if (err)
- return -EFAULT;
- break;
- case SIOCSHWTSTAMP:
- if (copy_from_user(&ifr, uifr32, sizeof(*uifr32)))
- return -EFAULT;
- ifr.ifr_data = compat_ptr(uifr32->ifr_ifru.ifru_data);
- break;
- default:
- if (copy_from_user(&ifr, uifr32, sizeof(*uifr32)))
- return -EFAULT;
- break;
- }
- old_fs = get_fs();
- set_fs (KERNEL_DS);
- err = sock_do_ioctl(net, sock, cmd, (unsigned long)&ifr);
- set_fs (old_fs);
+
+ uifr = compat_alloc_user_space(sizeof(*uifr));
+ if (copy_in_user(uifr, uifr32, sizeof(*uifr32)))
+ return -EFAULT;
+
+ err = sock_do_ioctl(net, sock, cmd, (unsigned long)uifr);
+
if (!err) {
switch (cmd) {
case SIOCGIFFLAGS:
@@ -2612,18 +2589,7 @@ static int dev_ifsioc(struct net *net, struct socket *sock,
case SIOCGIFTXQLEN:
case SIOCGMIIPHY:
case SIOCGMIIREG:
- if (copy_to_user(uifr32, &ifr, sizeof(*uifr32)))
- return -EFAULT;
- break;
- case SIOCGIFMAP:
- err = copy_to_user(uifr32, &ifr, sizeof(ifr.ifr_name));
- err |= __put_user(ifr.ifr_map.mem_start, &uifmap32->mem_start);
- err |= __put_user(ifr.ifr_map.mem_end, &uifmap32->mem_end);
- err |= __put_user(ifr.ifr_map.base_addr, &uifmap32->base_addr);
- err |= __put_user(ifr.ifr_map.irq, &uifmap32->irq);
- err |= __put_user(ifr.ifr_map.dma, &uifmap32->dma);
- err |= __put_user(ifr.ifr_map.port, &uifmap32->port);
- if (err)
+ if (copy_in_user(uifr32, uifr, sizeof(*uifr32)))
err = -EFAULT;
break;
}
@@ -2631,6 +2597,65 @@ static int dev_ifsioc(struct net *net, struct socket *sock,
return err;
}
+static int compat_sioc_ifmap(struct net *net, unsigned int cmd,
+ struct compat_ifreq __user *uifr32)
+{
+ struct ifreq ifr;
+ struct compat_ifmap __user *uifmap32;
+ mm_segment_t old_fs;
+ int err;
+
+ uifmap32 = &uifr32->ifr_ifru.ifru_map;
+ err = copy_from_user(&ifr, uifr32, sizeof(ifr.ifr_name));
+ err |= __get_user(ifr.ifr_map.mem_start, &uifmap32->mem_start);
+ err |= __get_user(ifr.ifr_map.mem_end, &uifmap32->mem_end);
+ err |= __get_user(ifr.ifr_map.base_addr, &uifmap32->base_addr);
+ err |= __get_user(ifr.ifr_map.irq, &uifmap32->irq);
+ err |= __get_user(ifr.ifr_map.dma, &uifmap32->dma);
+ err |= __get_user(ifr.ifr_map.port, &uifmap32->port);
+ if (err)
+ return -EFAULT;
+
+ old_fs = get_fs();
+ set_fs (KERNEL_DS);
+ err = dev_ioctl(net, cmd, (void __user *)&ifr);
+ set_fs (old_fs);
+
+ if (cmd == SIOCGIFMAP && !err) {
+ err = copy_to_user(uifr32, &ifr, sizeof(ifr.ifr_name));
+ err |= __put_user(ifr.ifr_map.mem_start, &uifmap32->mem_start);
+ err |= __put_user(ifr.ifr_map.mem_end, &uifmap32->mem_end);
+ err |= __put_user(ifr.ifr_map.base_addr, &uifmap32->base_addr);
+ err |= __put_user(ifr.ifr_map.irq, &uifmap32->irq);
+ err |= __put_user(ifr.ifr_map.dma, &uifmap32->dma);
+ err |= __put_user(ifr.ifr_map.port, &uifmap32->port);
+ if (err)
+ err = -EFAULT;
+ }
+ return err;
+}
+
+static int compat_siocshwtstamp(struct net *net, struct compat_ifreq __user *uifr32)
+{
+ void __user *uptr;
+ compat_uptr_t uptr32;
+ struct ifreq __user *uifr;
+
+ uifr = compat_alloc_user_space(sizeof (*uifr));
+ if (copy_in_user(uifr, uifr32, sizeof(struct compat_ifreq)))
+ return -EFAULT;
+
+ if (get_user(uptr32, &uifr32->ifr_data))
+ return -EFAULT;
+
+ uptr = compat_ptr(uptr32);
+
+ if (put_user(uptr, &uifr->ifr_data))
+ return -EFAULT;
+
+ return dev_ioctl(net, SIOCSHWTSTAMP, uifr);
+}
+
struct rtentry32 {
u32 rt_pad1;
struct sockaddr rt_dst; /* target address */
@@ -2923,6 +2948,9 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
return ethtool_ioctl(net, argp);
case SIOCWANDEV:
return compat_siocwandev(net, argp);
+ case SIOCGIFMAP:
+ case SIOCSIFMAP:
+ return compat_sioc_ifmap(net, cmd, argp);
case SIOCBONDENSLAVE:
case SIOCBONDRELEASE:
case SIOCBONDSETHWADDR:
@@ -2937,6 +2965,8 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
return do_siocgstamp(net, sock, cmd, argp);
case SIOCGSTAMPNS:
return do_siocgstampns(net, sock, cmd, argp);
+ case SIOCSHWTSTAMP:
+ return compat_siocshwtstamp(net, argp);
case FIOSETOWN:
case SIOCSPGRP:
@@ -2963,12 +2993,9 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
case SIOCADDMULTI:
case SIOCDELMULTI:
case SIOCGIFINDEX:
- case SIOCGIFMAP:
- case SIOCSIFMAP:
case SIOCGIFADDR:
case SIOCSIFADDR:
case SIOCSIFHWBROADCAST:
- case SIOCSHWTSTAMP:
case SIOCDIFADDR:
case SIOCGIFBRDADDR:
case SIOCSIFBRDADDR:
--
1.6.3.3
^ permalink raw reply related
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: Eric Dumazet @ 2009-11-10 14:29 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Network Developers, Ilpo Järvinen, Joe Perches
In-Reply-To: <4AF84BF3.5020302@gmail.com>
William Allen Simpson a écrit :
> This is a significantly revised implementation of an earlier (year-old)
> patch that no longer applies cleanly, with permission of the original
> author (Adam Langley). That patch was previously reviewed:
>
> http://thread.gmane.org/gmane.linux.network/102586
>
> The principle difference is using a TCP option to carry the cookie nonce,
> instead of a user configured offset in the data. This is more flexible and
> less subject to user configuration error. Such a cookie option has been
> suggested for many years, and is also useful without SYN data, allowing
> several related concepts to use the same extension option.
>
> "Re: SYN floods (was: does history repeat itself?)", September 9, 1996.
> http://www.merit.net/mail.archives/nanog/1996-09/msg00235.html
>
> "Re: what a new TCP header might look like", May 12, 1998.
> ftp://ftp.isi.edu/end2end/end2end-interest-1998.mail
>
> Data structures are carefully composed to require minimal additions.
> For example, the struct tcp_options_received cookie_plus variable fits
> between existing 16-bit and 8-bit variables, requiring no additional
> space (taking alignment into consideration). There are no additions to
> tcp_request_sock, and only 1 pointer in tcp_sock.
>
> Allocations have been rearranged to avoid requiring GFP_ATOMIC, with
> only one unavoidable exception in tcp_create_openreq_child(), where the
> tcp_sock itself is created GFP_ATOMIC.
>
> These functions will also be used in subsequent patches that implement
> additional features.
>
> Requires:
> TCPCT part 1a: add request_values parameter for sending SYNACK
> TCPCT part 1b: TCP_MSS_DEFAULT, TCP_MSS_DESIRED
> TCPCT part 1c: sysctl_tcp_cookie_size, socket option
> TCP_COOKIE_TRANSACTIONS, functions
> TCPCT part 1d: generate Responder Cookie
>
> Signed-off-by: William.Allen.Simpson@gmail.com
> ---
> include/linux/tcp.h | 29 ++++-
> include/net/tcp.h | 72 +++++++++++++
> net/ipv4/syncookies.c | 5 +-
> net/ipv4/tcp.c | 127 ++++++++++++++++++++++-
> net/ipv4/tcp_input.c | 86 +++++++++++++--
> net/ipv4/tcp_ipv4.c | 69 ++++++++++++-
> net/ipv4/tcp_minisocks.c | 59 ++++++++---
> net/ipv4/tcp_output.c | 255
> +++++++++++++++++++++++++++++++++++++++++-----
> net/ipv6/syncookies.c | 5 +-
> net/ipv6/tcp_ipv6.c | 65 +++++++++++-
> 10 files changed, 701 insertions(+), 71 deletions(-)
>
I really tried hard to understand what was going on, and failed, because I dont
have much time these days...
Lack of documentation maybe ? Some DATA flow could help...
Please please, cook up elementatry patches to perform one action at a time,
even if they are not fully functionnal ?
One patch to be able to send SYN + COOKIE (if we are the client)
One patch to be able to receive this SYN + COOKIE and answer a SYNACK + cookies (we are the server)
One patch to ... Receive the ACK from client (if we are the server) and check cookies
One patch to ... Send the ACK (if we are the client)
Patches to receive FIN + cookies
One patch to ... enable the whole thing and setsockopt() bits
That way we could review your patches step by step, and not 770 lines in one block.
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: Ilpo Järvinen @ 2009-11-10 14:30 UTC (permalink / raw)
To: William Allen Simpson
Cc: Linux Kernel Network Developers, Eric Dumazet, Joe Perches
In-Reply-To: <4AF96D71.8000905@gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 5192 bytes --]
On Tue, 10 Nov 2009, William Allen Simpson wrote:
> Ilpo Järvinen wrote:
> > On Mon, 9 Nov 2009, William Allen Simpson wrote:
> > > ...
> > > Data structures are carefully composed to require minimal additions.
> > > For example, the struct tcp_options_received cookie_plus variable fits
> > > between existing 16-bit and 8-bit variables, requiring no additional
> > > space (taking alignment into consideration). There are no additions to
> > > tcp_request_sock, and only 1 pointer in tcp_sock.
> > > ...
> >
> > One general comment. ...This particular patch still has lots of noise which
> > does not belong to the context of this change. ...Please try to minimize.
> > Eg., if you don't like sizeof(struct tcphdr) but prefer sizeof(*th), you
> > certainly don't have to do it in this particular patch!
After some time I returned to it. My apologies for my previous harsh
response. My point was to say that you must separate such changes because
they add to noise which makes it rather annoying to review. At least my
brains are rather limited still in keeping track of different things.
> This *is* actually in CodingStyle (line 679), and I'm trying to conform:
Sure, fell free to but not in some random places like you now do in this
patch. If you are not touching a line because of a real change and want do
a syntax cleanup, please do it in another patch. Especially when your
actual change is as complicated as this is.
> > ...Also some comment changes which certainly are not mandatory nor even
> > related.
> >
> Hmmm....
>
> 1) The first is a hole left by the removal of the data fields some time
> ago, but they left the old (now incorrect) comment:
>
> -/* SACKs data */
> + u8 cookie_plus:6, /* bytes in authenticator/cookie option */
> + cookie_out_never:1,
> + cookie_in_always:1;
> u8 num_sacks; /* Number of SACK blocks */
>
> Although it's not evident from the diff -u, I'm *filling* that hole,
> putting everything on a nice 32-bit boundary, thus taking *NO* space
> (already mentioned in my patch description above):
>
> u16 saw_tstamp : 1, /* Saw TIMESTAMP on last packet */
> tstamp_ok : 1, /* TIMESTAMP seen on SYN packet */
> dsack : 1, /* D-SACK is scheduled */
> wscale_ok : 1, /* Wscale seen on SYN packet */
> sack_ok : 4, /* SACK seen on SYN packet */
> snd_wscale : 4, /* Window scaling received from sender */
> rcv_wscale : 4; /* Window scaling to send to receiver */
> u8 cookie_plus:6, /* bytes in authenticator/cookie option */
> cookie_out_never:1,
> cookie_in_always:1;
> u8 num_sacks; /* Number of SACK blocks */
> u16 user_mss; /* mss requested by user in ioctl */
> u16 mss_clamp; /* Maximal mss, negotiated at connection setup
> */
>
> The only reason that it's done this way was Miller's imperative that
> required cramming this into as small a space as possible.
>
> "Store the data either somewhere else or in an extremely compact form."
>
> "Make your state take up less space in tcp_sock without making it cost
> more in some other form."
>
> Of course, it costs more, and I have to keep copying it from place to place,
> adding to the code complexity. But I was feeling rather clever to have
> found that hole!
I'm sorry but this response tells me that you don't seem to know even
yourself what you were submitting. Does this ring a bell:
- if (th->doff == sizeof(struct tcphdr) >> 2) {
+ /* In the spirit of fast parsing, compare doff directly to shifted
+ * constant values. Because equality is used, short doff can be
+ * ignored here, and checked later.
+ */
+ if (th->doff == (sizeof(*th) >> 2)) {
Besides, I don't understand even what you're trying to say. ...I think we
already checked the length in tcp_v[46]_rcv against underflow. Why you add
such a non-sense comment here (plus the sizeof change I covered
elsewhere)? This is just a noise to annoy reviewer, nothing else.
> 2) The second is a spelling error that I noticed in passing. It's within
> the same diff -u area (close to other insertions both before and after), so
> fixing it was trivial:
>
> - * to increse this, although since:
> + * to increase this, although since:
>
> Are we not supposed to fix spelling errors in comments?
How many extra lines a reviewer has to read because of that? Why not do it
in a separate patch. git add -i / git stash would help you there. ...To be
honest, I'd rather have typos in comments which like this are not
trouble for the reader than clutter feature patches with all this random
junk.
...I really wish that next time when you submit you get rid of all these
showstoppers and annoyances (ie., at least honestly try to do your best)
that we could finally concentrate to the actual change and reviewing
that :-). My recommendation is that before hitting the send button you
review (read through) your own patches, and if you find something to
correct instead of submitting, postpone that until the problem is solved.
Like DaveM once noted somebody, you'll not be timed :-). ...I usually do
that (read my own patches) and end up rather often to not submit (and find
even real bugs that way quite often).
--
i.
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 14:36 UTC (permalink / raw)
To: Gregory Haskins; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <4AF9747E.8020408@novell.com>
On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
> >>>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>> On Fri, Oct 02, 2009 at 10:20:00AM -0400, Gregory Haskins wrote:
> >>>> (Applies to davem/net-2.6.git:4fdb78d30)
> >>>>
> >>>> Hi David, netdevs,
> >>>>
> >>>> The following is an RFC for an attempt at addressing a zero-copy solution.
> >>>>
> >>>> To be perfectly honest, I have no idea if this is the best solution, or if
> >>>> there is truly a problem with skb->destructor that requires an alternate
> >>>> mechanism. What I do know is that this patch seems to work, and I would
> >>>> like to see some kind of solution available upstream. So I thought I would
> >>>> send my hack out as at least a point of discussion. FWIW: This has been
> >>>> tested heavily in my rig and is technically suitable for inclusion after
> >>>> review as is, if that is decided to be the optimal path forward here.
> >>>>
> >>>> Thanks for your review and consideration,
> >>>>
> >>>> Kind regards,
> >>>> -Greg
> >>>>
> >>>> ----------------------------------------
> >>>> From: Gregory Haskins <ghaskins@novell.com>
> >>>> Subject: [RFC PATCH] net: add dataref destructor to sk_buff
> >>>>
> >>>> What: The skb->destructor field is reportedly unreliable for ensuring
> >>>> that all shinfo users have dropped their references. Therefore, we add
> >>>> a distinct ->release() method for the shinfo structure which is closely
> >>>> tied to the underlying page resources we want to protect.
> >>>>
> >>>> Why: We want to add zero-copy transmit support for AlacrityVM guests.
> >>>> In order to support this, the host kernel must map guest pages directly
> >>>> into a paged-skb and send it as normal. put_page() alone is not
> >>>> sufficient lifetime management since the pages are ultimately allocated
> >>>> from within the guest. Therefore, we need higher-level notification
> >>>> when the skb is finally freed on the host so we can then inject a proper
> >>>> "tx-complete" event into the guest context.
> >>>>
> >>>> Signed-off-by: Gregory Haskins <ghaskins@novell.com>
> >>>> ---
> >>>>
> >>>> include/linux/skbuff.h | 2 ++
> >>>> net/core/skbuff.c | 9 +++++++++
> >>>> 2 files changed, 11 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> >>>> index df7b23a..02cdab6 100644
> >>>> --- a/include/linux/skbuff.h
> >>>> +++ b/include/linux/skbuff.h
> >>>> @@ -207,6 +207,8 @@ struct skb_shared_info {
> >>>> /* Intermediate layers must ensure that destructor_arg
> >>>> * remains valid until skb destructor */
> >>>> void * destructor_arg;
> >>>> + void * priv;
> >>>> + void (*release)(struct sk_buff *skb);
> >>>> };
> >>>>
> >>>> /* We divide dataref into two halves. The higher 16 bits hold references
> >>>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >>>> index 80a9616..a7e40a9 100644
> >>>> --- a/net/core/skbuff.c
> >>>> +++ b/net/core/skbuff.c
> >>>> @@ -219,6 +219,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t
> >>> gfp_mask,
> >>>> shinfo->tx_flags.flags = 0;
> >>>> skb_frag_list_init(skb);
> >>>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> >>>> + shinfo->release = NULL;
> >>>> + shinfo->priv = NULL;
> >>>>
> >>>> if (fclone) {
> >>>> struct sk_buff *child = skb + 1;
> >>>> @@ -350,6 +352,9 @@ static void skb_release_data(struct sk_buff *skb)
> >>>> if (skb_has_frags(skb))
> >>>> skb_drop_fraglist(skb);
> >>>>
> >>>> + if (skb_shinfo(skb)->release)
> >>>> + skb_shinfo(skb)->release(skb);
> >>>> +
> >>>> kfree(skb->head);
> >>>> }
> >>>> }
> >>>> @@ -514,6 +519,8 @@ int skb_recycle_check(struct sk_buff *skb, int skb_size)
> >>>> shinfo->tx_flags.flags = 0;
> >>>> skb_frag_list_init(skb);
> >>>> memset(&shinfo->hwtstamps, 0, sizeof(shinfo->hwtstamps));
> >>>> + shinfo->release = NULL;
> >>>> + shinfo->priv = NULL;
> >>>>
> >>>> memset(skb, 0, offsetof(struct sk_buff, tail));
> >>>> skb->data = skb->head + NET_SKB_PAD;
> >>>> @@ -856,6 +863,8 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
> >>> ntail,
> >>>> skb->hdr_len = 0;
> >>>> skb->nohdr = 0;
> >>>> atomic_set(&skb_shinfo(skb)->dataref, 1);
> >>>> + skb_shinfo(skb)->release = NULL;
> >>>> + skb_shinfo(skb)->priv = NULL;
> >>>> return 0;
> >>>>
> >>>> nodata:
> >>> This is basically subset of the skb data destructors patch, isn't it?
> >> Sort of, but the emphasis is different. Here are the main differences:
> >>
> >> 1) skb->destructor() is on the skb level, shinfo->release() is at the shared-info/page level
> >>
> >> 2) skb->destructor is (iiuc) modified during the skb's lifetime (for instance rmem hooks here to
> >> manage the buffer-space dynamically), whereas shinfo->release is designed to be used by
> >> "the owner" and is thus only set at creation.
> >>
> >> 3) shinfo->release tracks the lifetime of the pages, not the skb. This means it transcends
> >> the skb's lifetime (such as splits for clone, etc) and focuses on the shared component: the pages
> >
> > You are comparing with skb destructors, not skb data destructors :) skb
> > data destructor is Rusty's patch which he wanted to use for vringfd. I
> > mean e.g. this:
> > http://lists.openwall.net/netdev/2008/04/18/7
>
> Ah, I wasn't aware. I believe Anthony Ligouri had pointed me at this
> same patch earlier this year. However, more recently when I saw
> skb->destructor() in mainline (seemingly from Rusty), I thought it had
> been accepted and didn't investigate further. I see now that you are
> talking about something else.
>
> >
> >>> Last time this was tried, this is the objection that was voiced:
> >>>
> >>> The problem with this patch is that it's tracking skb's, while
> >>> you want use it to track pages for zero-copy. That just doesn't
> >>> work. Through mechanisms like splice, individual pages in the
> >>> skb can be detached and metastasize to other locations, e.g.,
> >>> the VFS.
> >> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
> >> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
> >
> > VFS does not know about shinfo either, does it?
>
> I do not follow the reference. Where does VFS come into play?
"Through mechanisms like splice, individual pages in the
skb can be detached and metastasize to other locations, e.g.,
the VFS"
> >
> >>> and I think this applies here.
> >> I don't think so, but if you think I missed something, do not be shy (not that you ever are).
> >
> > Well, I hope the reviews are helpful.
>
> Yes, I didn't mean it as a slam. Was only trying to convey that I
> didn't think I was wrong but am open minded to the possibility, so
> please keep the discussion going.
>
> > I'll be happy if we learn to
> > track pages involved in transmit, but need to be careful.
> >
>
> Agreed.
>
> >>> In other words, this only *seems*
> >>> to work for you because you are not trying to do things like
> >>> guest to host communication, with host doing smart things.
> >> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
> >> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
> >> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
> >> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
> >
> > Not if someone else is referencing the pages without a reference to shinfo.
>
> I agree that if we can reference pages outside of the skb/shinfo then
> there is a problem. I wasn't aware that we could do this, tbh.
>
> However, it seems to me that this is a problem with the overall stack,
> if true....isn't it? For instance, if I do a sendmsg() from a userspace
> app and block until its consumed,
consumed == memcpy_from_iovec?
> how can the system function sanely if
> the app returns from the call but something is still referencing the
> page(s)?
which pages?
> This seems broken to me.
> >
> >>> Cc Herbert which was involved in the original discussion.
> >>>
> >>> In the specific case, it seems that things like pskb_copy,
> >>> skb_split and others will also be broken, won't they?
> >> Not to my knowledge. They up the reference to the shinfo before proceeding.
> >
> > I don't seem to find where does skb_split reference the shinfo.
> > It seems to do get_page on individual pages?
>
> Ill take a look.
>
> If so, one alternate solution that I had considered was to look at some
> kind of page->release() hook. There are obvious disadvantages to such a
> solution (for one, it has no such notion, and second we have to manage
> the aggregate collection which has overhead), so I shied away from that
> design in favor of this one. But perhaps we will ultimately have no choice.
>
> Kind Regards,
> -Greg
>
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 3/5 RFC] TCPCT part1c: sysctl_tcp_cookie_size, socket option TCP_COOKIE_TRANSACTIONS, functions
From: William Allen Simpson @ 2009-11-10 14:43 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: Linux Kernel Network Developers
In-Reply-To: <alpine.DEB.2.00.0911100706120.7059@melkinpaasi.cs.helsinki.fi>
Ilpo Järvinen wrote:
> I guess most of the cookie stuff have nothing to do with the next,
> please make them separate:
>
>> Redefine two TCP header functions to accept TCP header pointer.
>> When subtracting, return signed int to allow error checking.
>
> Convert the users here, not in the fifth patch to avoid noise in the large
> fifth patch.
>
These comments are directly contrary to advice back on the 2nd or 3rd
round, where I'd both converted and removed the old functions here, before
going on to use them everywhere. I was told to *only* define the new ones
here, use them in later patches as they applied, and then remove the old
functions in a final cleanup patch.
When I'm done (apparently very far in the distant future), there will be
approximately 100 (maybe 150 or more) uses.
> And, I read more that fifth patch... seriously, please consider now to
> apply _all_ the coding style changes that have been brought to your
> attention by multiple people (you should know them now but for some reason
> again you choose to resent without complying -- I guess on purpose) into
> all upcoming patches you submit.
>
Pardon me, but I'm having difficulty finding a substantive comment. Are
you attempting humor or sarcasm? If so, it's not readily apparent. It
seems more of an /ad hominem/ attack to me.
There were many painstaking hours of coding style changes, every single
one of them had to be individually examined side by side for correctness
after I ran a series of regexp over the patches -- in 148 blocks (I just
checked) of diff between the patch diffs, comprising eye-glazing trivial
changes (although I used BBEdit's very nice GUI utility for the
side-by-side comparison).
If you don't like something else, please be specific (as others did
privately). There may be places the regexp missed.
But more importantly, I'd prefer actual analysis. Is there a better
Linux function to use for something (like RCU that Eric recommended)?
This is the *easy* patch series, based on code previously reviewed. The
next series are much more conceptually difficult. Maybe that's the
problem, this is too easy, and so the only things people find to comment
about are trivia.
Please, more thoughtful code review, less interpersonal sniping.
TIA.
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 14:45 UTC (permalink / raw)
To: Gregory Haskins; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <4AF9747E.8020408@novell.com>
On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
> > You are comparing with skb destructors, not skb data destructors :) skb
> > data destructor is Rusty's patch which he wanted to use for vringfd. I
> > mean e.g. this:
> > http://lists.openwall.net/netdev/2008/04/18/7
>
> Ah, I wasn't aware. I believe Anthony Ligouri had pointed me at this
> same patch earlier this year. However, more recently when I saw
> skb->destructor() in mainline (seemingly from Rusty), I thought it had
> been accepted and didn't investigate further.
skb->destructor seems to be there as far back as 2.2.0
http://lxr.linux.no/#linux-old+v2.2.0/include/linux/skbuff.h
--
MST
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Jean Delvare @ 2009-11-10 15:02 UTC (permalink / raw)
To: Ben Hutchings
Cc: Stephen Rothwell, David Miller, netdev, linux-next, linux-kernel,
Mika Kuoppala
In-Reply-To: <1257859379.2834.2.camel@achroite.uk.solarflarecom.com>
On Tue, 10 Nov 2009 13:22:58 +0000, Ben Hutchings wrote:
> On Tue, 2009-11-10 at 12:42 +0100, Jean Delvare wrote:
> > Ben, you can adjust your own patches to make use of this API instead of
> > accessing the i2c_adapter mutex directly. That way, you are no longer
> > dependent of implementation changes, and this should solve the conflict.
> >
> > Stephen, you can then drop your fixup patch.
>
> I don't think so, since the conflict resulted from joining two files
> including sfe4001.c in net-next-2.6.
My patch series no longer touches sfe4001.c, so how would the conflict
remain?
--
Jean Delvare
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Ben Hutchings @ 2009-11-10 15:10 UTC (permalink / raw)
To: Jean Delvare, David Miller
Cc: Stephen Rothwell, netdev, linux-next, linux-kernel, Mika Kuoppala
In-Reply-To: <20091110160246.506b7789@hyperion.delvare>
On Tue, 2009-11-10 at 16:02 +0100, Jean Delvare wrote:
> On Tue, 10 Nov 2009 13:22:58 +0000, Ben Hutchings wrote:
> > On Tue, 2009-11-10 at 12:42 +0100, Jean Delvare wrote:
> > > Ben, you can adjust your own patches to make use of this API instead of
> > > accessing the i2c_adapter mutex directly. That way, you are no longer
> > > dependent of implementation changes, and this should solve the conflict.
> > >
> > > Stephen, you can then drop your fixup patch.
> >
> > I don't think so, since the conflict resulted from joining two files
> > including sfe4001.c in net-next-2.6.
>
> My patch series no longer touches sfe4001.c, so how would the conflict
> remain?
Because your patch to introduce i2c_{lock,unlock}_adapter() are not in
net-next-2.6 yet.
David, you might want to pull from Linus and resolve this, giving
Stephen a break.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 15:45 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110143652.GB19645@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4225 bytes --]
Michael S. Tsirkin wrote:
> On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
>> Michael S. Tsirkin wrote:
>>> On Tue, Nov 10, 2009 at 05:40:50AM -0700, Gregory Haskins wrote:
>>>>>>> On 11/10/2009 at 6:53 AM, in message <20091110115335.GC6989@redhat.com>,
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>>>> Last time this was tried, this is the objection that was voiced:
>>>>>
>>>>> The problem with this patch is that it's tracking skb's, while
>>>>> you want use it to track pages for zero-copy. That just doesn't
>>>>> work. Through mechanisms like splice, individual pages in the
>>>>> skb can be detached and metastasize to other locations, e.g.,
>>>>> the VFS.
>>>> Right, and I don't think this applies here because I specifically chose the shinfo level to try to properly
>>>> track the page level avoid this issue. Multiple skb's can point to a single shinfo, iiuc.
>>> VFS does not know about shinfo either, does it?
>> I do not follow the reference. Where does VFS come into play?
>
> "Through mechanisms like splice, individual pages in the
> skb can be detached and metastasize to other locations, e.g.,
> the VFS"
Right, understood. What I mean is: How is that actually used in
real-life in a way that is valid?
What I am getting at is as follows: From a real basic perspective, you
can look at all of this as a simple synchronous call (i.e. sendmsg()).
The "app" (be it a userspace app, or a guest) prepares a buffer for
transmission, and offers it to the next layer in the stack. The app
must maintain the integrity of that buffer at least until the layer
below it signifies that it is "consumed". This may mean its a
synchronous call, like sendmsg(), or it may be asynchronous, like AIO.
But the key thing here is that at some point, the lower layer has to
signify that the buffer stability constraint has been met. In either
case, we have a clear delineated event: the io-completes = the buffer is
free to be reused.
In the simple case, the buffer in question is copied to a kernel buffer,
and the io completes immediately. In other cases (such as zero copy),
the buffer is mapped into the skb, and we have to wait for even lower
layers to signify the completion.
I am not a stack expert, but I was under the impression that we use this
model for userspace pages today as well using the wmem callbacks in
skb->destructor(). If so, I do not see how you could do something like
detach a page from a pskb and still expect to have a proper event that
delineates the io-completion to the higher layers.
So the questions are:
1) do we in fact map userspace pages to pskbs today?
2a) if so, how do we delineate the completion event?
2b) and how do we prevent worrying about the get_page() issue you refer
to.
>>
>>>>> In other words, this only *seems*
>>>>> to work for you because you are not trying to do things like
>>>>> guest to host communication, with host doing smart things.
>>>> I am not following what you mean here, as I do use this for guest->host and guest->host->remote, and
>>>> it works quite nicely. I map the guest pages in, and when the last reference to the pages are dropped,
>>>> I release the pages back to the guest. It doesn't matter if the skb egresses out a physical adapter or is
>>>> received locally. All that matters is the lifetime of the shinfo (and thus its pages) is handled correctly.
>>> Not if someone else is referencing the pages without a reference to shinfo.
>> I agree that if we can reference pages outside of the skb/shinfo then
>> there is a problem. I wasn't aware that we could do this, tbh.
>>
>> However, it seems to me that this is a problem with the overall stack,
>> if true....isn't it? For instance, if I do a sendmsg() from a userspace
>> app and block until its consumed,
>
> consumed == memcpy_from_iovec?
For non-zero-copy, sure why not.
>
>> how can the system function sanely if
>> the app returns from the call but something is still referencing the
>> page(s)?
>
> which pages?
You said that there are paths that get_page() out of shinfo without
holding a shinfo reference.
Kind Regards,
-Greg
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange with SYNACK data
From: William Allen Simpson @ 2009-11-10 15:45 UTC (permalink / raw)
To: Eric Dumazet
Cc: Linux Kernel Network Developers, Ilpo Järvinen, Joe Perches
In-Reply-To: <4AF978D3.7030207@gmail.com>
Eric Dumazet wrote:
> William Allen Simpson a écrit :
>> This is a significantly revised implementation of an earlier (year-old)
>> patch that no longer applies cleanly, with permission of the original
>> author (Adam Langley). That patch was previously reviewed:
>>
>> http://thread.gmane.org/gmane.linux.network/102586
> I really tried hard to understand what was going on, and failed, because I dont
> have much time these days...
>
Thanks very much for your time.
As I just wrote in a previous message, this is the easy part. It's going
to get much more complicated -- as you know, since I sent you the papers
with 30+ references....
> Lack of documentation maybe ? Some DATA flow could help...
>
Where should that be? In the commit messages?
> Please please, cook up elementatry patches to perform one action at a time,
> even if they are not fully functionnal ?
>
Well, that's what I did with the (now) parts 1b, 1c, and 1d, but somebody
complained that the callers didn't exist yet.
> One patch to be able to send SYN + COOKIE (if we are the client)
>
> One patch to be able to receive this SYN + COOKIE and answer a SYNACK + cookies (we are the server)
>
I'll split them. Remember, I started with a code base previously
submitted, and everything was in one *single* large patch. I thought
that's what you (Linux folks in general) wanted.
> One patch to ... Receive the ACK from client (if we are the server) and check cookies
>
> One patch to ... Send the ACK (if we are the client)
>
> Patches to receive FIN + cookies
>
Maybe that's what you are missing, and why you're have difficulty
wrapping your mind around it. None of that is there yet!
Receive the Ack options from the client is currently part 2. I've also
lately divided part 2 into 4 sub-patches, although it all has to be
committed at one time:
2a) extending the TCP option space,
2b) receiving 64-bit timestamps,
2c) receiving extended SACKs,
2d) receiving the cookie pair.
Perhaps that could be split even further, although the order has to
stay the same. A lot is written, although I gave up updating regularly
as the part 1 process has taken so long....
Send the Ack is currently part 3.
Handling FIN is currently part 4, although it might be split, too.
Handling RST is probably part 5.
Handling transactions was part 4 or 5, but could be a future part 6
instead (although it's rather tied up with both 4 and 5).
> One patch to ... enable the whole thing and setsockopt() bits
>
Ummm, then there'd be no way to test. This is TCP we're talking about.
It has to be tested extensively. This stuff needs to work. I need the
bits to initiate all the tests....
That's why I like to get all my data structures nailed down first, and
then incrementally add code and test.
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Gregory Haskins @ 2009-11-10 15:47 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: alacrityvm-devel, herbert.xu, linux-kernel, netdev
In-Reply-To: <20091110144543.GC19645@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 806 bytes --]
Michael S. Tsirkin wrote:
> On Tue, Nov 10, 2009 at 09:11:10AM -0500, Gregory Haskins wrote:
>>> You are comparing with skb destructors, not skb data destructors :) skb
>>> data destructor is Rusty's patch which he wanted to use for vringfd. I
>>> mean e.g. this:
>>> http://lists.openwall.net/netdev/2008/04/18/7
>> Ah, I wasn't aware. I believe Anthony Ligouri had pointed me at this
>> same patch earlier this year. However, more recently when I saw
>> skb->destructor() in mainline (seemingly from Rusty), I thought it had
>> been accepted and didn't investigate further.
>
> skb->destructor seems to be there as far back as 2.2.0
> http://lxr.linux.no/#linux-old+v2.2.0/include/linux/skbuff.h
>
Right, I meant recent activity around it for fixing it up related to
leaks, etc.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 267 bytes --]
^ permalink raw reply
* net 01/02: allow to propagate errors through ->ndo_hard_start_xmit()
From: Patrick McHardy @ 2009-11-10 16:14 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
[-- Attachment #1: Type: text/plain, Size: 438 bytes --]
The following two patches add support for error propagation
through ->ndo_hard_start_xmit() and use it to propagate qdisc
submission state through VLAN and macvlan devices.
I'm also experimenting with returning errno codes from
tunnel devices to the originating socket, but those patches
still need some work.
The only change relative to the patch posted yesterday is
removal of an unnecessary initialization in dev_hard_start_xmit().
[-- Attachment #2: 01.diff --]
[-- Type: text/x-patch, Size: 6241 bytes --]
commit ebf56943a164bfee312844bffff8f6b6ff8183dc
Author: Patrick McHardy <kaber@trash.net>
Date: Tue Nov 10 16:55:45 2009 +0100
net: allow to propagate errors through ->ndo_hard_start_xmit()
Currently the ->ndo_hard_start_xmit() callbacks are only permitted to return
one of the NETDEV_TX codes. This prevents any kind of error propagation for
virtual devices, like queue congestion of the underlying device in case of
layered devices, or unreachability in case of tunnels.
This patches changes the NET_XMIT codes to avoid clashes with the NETDEV_TX
codes and changes the two callers of dev_hard_start_xmit() to expect either
errno codes, NET_XMIT codes or NETDEV_TX codes as return value.
In case of qdisc_restart(), all non NETDEV_TX codes are mapped to NETDEV_TX_OK
since no error propagation is possible when using qdiscs. In case of
dev_queue_xmit(), the error is propagated upwards.
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 465add6..ab2812c 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -63,27 +63,48 @@ struct wireless_dev;
#define HAVE_FREE_NETDEV /* free_netdev() */
#define HAVE_NETDEV_PRIV /* netdev_priv() */
-#define NET_XMIT_SUCCESS 0
-#define NET_XMIT_DROP 1 /* skb dropped */
-#define NET_XMIT_CN 2 /* congestion notification */
-#define NET_XMIT_POLICED 3 /* skb is shot by police */
-#define NET_XMIT_MASK 0xFFFF /* qdisc flags in net/sch_generic.h */
+/*
+ * Transmit return codes: transmit return codes originate from three different
+ * namespaces:
+ *
+ * - qdisc return codes
+ * - driver transmit return codes
+ * - errno values
+ *
+ * Drivers are allowed to return any one of those in their hard_start_xmit()
+ * function. Real network devices commonly used with qdiscs should only return
+ * the driver transmit return codes though - when qdiscs are used, the actual
+ * transmission happens asynchronously, so the value is not propagated to
+ * higher layers. Virtual network devices transmit synchronously, in this case
+ * the driver transmit return codes are consumed by dev_queue_xmit(), all
+ * others are propagated to higher layers.
+ */
+
+/* qdisc ->enqueue() return codes. */
+#define NET_XMIT_SUCCESS 0x00
+#define NET_XMIT_DROP 0x10 /* skb dropped */
+#define NET_XMIT_CN 0x20 /* congestion notification */
+#define NET_XMIT_POLICED 0x30 /* skb is shot by police */
+#define NET_XMIT_MASK 0xf0 /* qdisc flags in net/sch_generic.h */
/* Backlog congestion levels */
-#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
-#define NET_RX_DROP 1 /* packet dropped */
+#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
+#define NET_RX_DROP 1 /* packet dropped */
/* NET_XMIT_CN is special. It does not guarantee that this packet is lost. It
* indicates that the device will soon be dropping packets, or already drops
* some packets of the same priority; prompting us to send less aggressively. */
-#define net_xmit_eval(e) ((e) == NET_XMIT_CN? 0 : (e))
+#define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e))
#define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0)
/* Driver transmit return codes */
+#define NETDEV_TX_MASK 0xf
+
enum netdev_tx {
- NETDEV_TX_OK = 0, /* driver took care of packet */
- NETDEV_TX_BUSY, /* driver tx path was busy*/
- NETDEV_TX_LOCKED = -1, /* driver tx lock was already taken */
+ __NETDEV_TX_MIN = INT_MIN, /* make sure enum is signed */
+ NETDEV_TX_OK = 0, /* driver took care of packet */
+ NETDEV_TX_BUSY = 1, /* driver tx path was busy*/
+ NETDEV_TX_LOCKED = 2, /* driver tx lock was already taken */
};
typedef enum netdev_tx netdev_tx_t;
diff --git a/net/core/dev.c b/net/core/dev.c
index bf629ac..babd45f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1756,7 +1756,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
struct netdev_queue *txq)
{
const struct net_device_ops *ops = dev->netdev_ops;
- int rc;
+ int rc = NETDEV_TX_OK;
if (likely(!skb->next)) {
if (!list_empty(&ptype_all))
@@ -1804,6 +1804,8 @@ gso:
nskb->next = NULL;
rc = ops->ndo_start_xmit(nskb, dev);
if (unlikely(rc != NETDEV_TX_OK)) {
+ if (rc & ~NETDEV_TX_MASK)
+ goto out_kfree_gso_skb;
nskb->next = skb->next;
skb->next = nskb;
return rc;
@@ -1813,11 +1815,12 @@ gso:
return NETDEV_TX_BUSY;
} while (skb->next);
- skb->destructor = DEV_GSO_CB(skb)->destructor;
-
+out_kfree_gso_skb:
+ if (likely(skb->next == NULL))
+ skb->destructor = DEV_GSO_CB(skb)->destructor;
out_kfree_skb:
kfree_skb(skb);
- return NETDEV_TX_OK;
+ return rc;
}
static u32 skb_tx_hashrnd;
@@ -1905,6 +1908,23 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q,
return rc;
}
+static inline bool dev_xmit_complete(int rc)
+{
+ /* successful transmission */
+ if (rc == NETDEV_TX_OK)
+ return true;
+
+ /* error while transmitting, driver consumed skb */
+ if (rc < 0)
+ return true;
+
+ /* error while queueing to a different device, driver consumed skb */
+ if (rc & NET_XMIT_MASK)
+ return true;
+
+ return false;
+}
+
/**
* dev_queue_xmit - transmit a buffer
* @skb: buffer to transmit
@@ -2002,8 +2022,8 @@ gso:
HARD_TX_LOCK(dev, txq, cpu);
if (!netif_tx_queue_stopped(txq)) {
- rc = NET_XMIT_SUCCESS;
- if (!dev_hard_start_xmit(skb, dev, txq)) {
+ rc = dev_hard_start_xmit(skb, dev, txq);
+ if (dev_xmit_complete(rc)) {
HARD_TX_UNLOCK(dev, txq);
goto out;
}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 4ae6aa5..b13821a 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -120,8 +120,15 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
HARD_TX_LOCK(dev, txq, smp_processor_id());
if (!netif_tx_queue_stopped(txq) &&
- !netif_tx_queue_frozen(txq))
+ !netif_tx_queue_frozen(txq)) {
ret = dev_hard_start_xmit(skb, dev, txq);
+
+ /* an error implies that the skb was consumed */
+ if (ret < 0)
+ ret = NETDEV_TX_OK;
+ /* all NET_XMIT codes map to NETDEV_TX_OK */
+ ret &= ~NET_XMIT_MASK;
+ }
HARD_TX_UNLOCK(dev, txq);
spin_lock(root_lock);
^ permalink raw reply related
* vlan/macvlan 02/02: propagate transmission state to upper layers
From: Patrick McHardy @ 2009-11-10 16:14 UTC (permalink / raw)
To: David S. Miller; +Cc: Linux Netdev List
[-- Attachment #1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2: 02.diff --]
[-- Type: text/x-patch, Size: 1514 bytes --]
commit 21fb413b55abb4665e057b884c289bb780f41b3c
Author: Patrick McHardy <kaber@trash.net>
Date: Tue Nov 10 16:55:50 2009 +0100
vlan/macvlan: propagate transmission state to upper layers
Both vlan and macvlan devices usually don't use a qdisc and immediately
queue packets to the underlying device. Propagate transmission state of
the underlying device to the upper layers so they can react on congestion
and/or inform the sending process.
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index d7dba3f..271aa7e 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -202,7 +202,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
} else
txq->tx_dropped++;
- return NETDEV_TX_OK;
+ return ret;
}
static int macvlan_hard_header(struct sk_buff *skb, struct net_device *dev,
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 790fd55..9159659 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -332,7 +332,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
} else
txq->tx_dropped++;
- return NETDEV_TX_OK;
+ return ret;
}
static netdev_tx_t vlan_dev_hwaccel_hard_start_xmit(struct sk_buff *skb,
@@ -358,7 +358,7 @@ static netdev_tx_t vlan_dev_hwaccel_hard_start_xmit(struct sk_buff *skb,
} else
txq->tx_dropped++;
- return NETDEV_TX_OK;
+ return ret;
}
static int vlan_dev_change_mtu(struct net_device *dev, int new_mtu)
^ permalink raw reply related
* Bridge + Conntrack + SKB Recycle: Fragment Reassembly Errors
From: ben @ 2009-11-10 16:09 UTC (permalink / raw)
To: netdev
We have observed significant reassembly errors when combining
routing/bridging with conntrack + nf_defrag_ipv4 loaded, and
skb_recycle_check - enabled interfaces. For our test, we had a single
linux device with two interfaces (gianfars in this case) with SKB
recycling enabled. We sent large, continuous pings across the bridge,
like this:
ping -s 64000 -A <dest IP>
Then, we ran netstat -s --raw, and noticed that IPSTATS_MIB_REASMFAILS
were happening for about 40% of the received datagrams. Tracing the
code in ip_fragment.c, we instrumented each of the
IPSTATS_MIB_REASMFAILS locations, and found the culprit to be
ip_evictor. Nothing looked unusual here, so we placed tracing in
ip_frag_queue, directly above:
atomic_add(skb->truesize, &qp->q.net->mem);
We noticed that quite a few of the skb->truesize numbers were in the 67K
range, which quickly overwhelms the default 192K-ish ipfrag_low_thresh.
This means that the next time inet_frag_evictor is run:
work = atomic_read(&nf->mem) - nf->low_thresh;
Will surely be positive, and it is likely that our huge-frag-containing
queue will be one of those evicted.
Looking at the source of these huge skbs, it seems that during
re-fragmentation in br_nf_dev_queue_xmit (which calls ip_fragment with
CONFIG_NF_CONNTRACK_IPV4 enabled), the huge datagram that was allocated
to hold a successfully-reassembled skb may be getting reused? In any
case, when skb_recycle_check(skb, min_rx_size) is called, the huge
(skb->truesize huge, not data huge) skb is recycled for use on RX, and
it eventually gets enqueued for reassembly, causing the
inet_frag_evictor to have a positive work value.
Our solution was to add an upper-bounds check to skb_recycle_check,
which prevents the large-ish SKBs from being used to create future
frags, and overwhelming ipfrag_low_thresh. This seems quite clunky,
although I would be happy to submit this as a patch...
If this is not the right place...what is the "right" place?
Ben Menchaca
Bigfoot Networks
^ 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