* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 11:41 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100314i33d6a49dl5163b5e8472babcf@mail.gmail.com>
Changli Gao wrote:
> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>> Changli Gao a écrit :
>>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>>> Not sure how to solve this problem (several cpus can updates counter in //)
>>>>
>>> Thanks, and follow your suggestions. I can maintain the counter per TX
>>> queue, and update it in a timer handler like ixgbe or implement our
>>> own struct net_device_stats* (*ndo_get_stats)(struct net_device *dev),
>>> and update the counters when it gets called.
>> Please no timer stuff :)
>
> The whole ifb.c file is attached, please review and test it. Thanks!
> /* Number of TX queues per ifb */
> static int numtxqs = 1;
> module_param(numtxqs, int, 0444);
> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
Module parameters suck as a configuration API. The existing numifbs
option exists purely for compatibility reasons, I'd prefer if you'd
this to the netlink interface.
> static int __init ifb_init_one(int index)
> {
> struct net_device *dev_ifb;
> int err;
>
> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
> ifb_setup, numtxqs);
>
This won't work for the rtnl_link setup since the size must
be constant.
^ permalink raw reply
* Re: linux-next: manual merge of the net tree with the i2c tree
From: Jean Delvare @ 2009-11-10 11:42 UTC (permalink / raw)
To: Stephen Rothwell, Ben Hutchings
Cc: David Miller, netdev, linux-next, linux-kernel, Mika Kuoppala
In-Reply-To: <20091026133757.7cf87e49.sfr@canb.auug.org.au>
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
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.
Thanks,
--
Jean Delvare
^ permalink raw reply
* Re: [RFC PATCH] net: add dataref destructor to sk_buff
From: Michael S. Tsirkin @ 2009-11-10 11:53 UTC (permalink / raw)
To: Gregory Haskins; +Cc: netdev, linux-kernel, herbert.xu
In-Reply-To: <20091002141407.30224.54207.stgit@dev.haskins.net>
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?
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.
and I think this applies here. 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.
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?
--
MST
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 12:14 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF95174.3060104@trash.net>
On Tue, Nov 10, 2009 at 7:41 PM, Patrick McHardy <kaber@trash.net> wrote:
> Changli Gao wrote:
>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>
>> The whole ifb.c file is attached, please review and test it. Thanks!
>
>> /* Number of TX queues per ifb */
>> static int numtxqs = 1;
>> module_param(numtxqs, int, 0444);
>> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>
> Module parameters suck as a configuration API. The existing numifbs
> option exists purely for compatibility reasons, I'd prefer if you'd
> this to the netlink interface.
How to do that? I haven't found any examples. Is there a interface to
config the number of the TX queues of a NIC.
>
>> static int __init ifb_init_one(int index)
>> {
>> struct net_device *dev_ifb;
>> int err;
>>
>> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
>> ifb_setup, numtxqs);
>>
>
> This won't work for the rtnl_link setup since the size must
> be constant.
>
Does this work?
ifb_link_ops.priv_size = sizeof(struct ifb_private) * numtxqs;
rtnl_lock();
err = __rtnl_link_register(&ifb_link_ops);
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Patrick McHardy @ 2009-11-10 12:19 UTC (permalink / raw)
To: Changli Gao; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <412e6f7f0911100414w7b6f45aap39f6568b7af2c1a7@mail.gmail.com>
Changli Gao wrote:
> On Tue, Nov 10, 2009 at 7:41 PM, Patrick McHardy <kaber@trash.net> wrote:
>> Changli Gao wrote:
>>> 2009/11/10 Eric Dumazet <eric.dumazet@gmail.com>:
>>>
>>> The whole ifb.c file is attached, please review and test it. Thanks!
>>> /* Number of TX queues per ifb */
>>> static int numtxqs = 1;
>>> module_param(numtxqs, int, 0444);
>>> MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>> Module parameters suck as a configuration API. The existing numifbs
>> option exists purely for compatibility reasons, I'd prefer if you'd
>> this to the netlink interface.
>
> How to do that? I haven't found any examples. Is there a interface to
> config the number of the TX queues of a NIC.
You have to add a get_tx_queues() callback to the rtnl_link_ops.
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.
>>> static int __init ifb_init_one(int index)
>>> {
>>> struct net_device *dev_ifb;
>>> int err;
>>>
>>> dev_ifb = alloc_netdev_mq(sizeof(struct ifb_private) * numtxqs, "ifb%d",
>>> ifb_setup, numtxqs);
>>>
>> This won't work for the rtnl_link setup since the size must
>> be constant.
>>
>
> 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.
^ permalink raw reply
* Re: [PATCH] ifb: add multi-queue support
From: Changli Gao @ 2009-11-10 12:37 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Eric Dumazet, David S. Miller, netdev, Tom Herbert
In-Reply-To: <4AF95A3D.5060602@trash.net>
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.
> 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.
>> 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?
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* 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
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