* Re: [PATCH v3] ipconfig wait for carrier
From: Micha Nelissen @ 2011-05-18 6:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20110518.023725.441358922110721042.davem@davemloft.net>
Op 2011-05-18 8:37, David Miller schreef:
> From: Micha Nelissen<micha@neli.hopto.org>
> Date: Wed, 18 May 2011 08:32:35 +0200
>
>> I'm confused. Against which tree/commit do you want it then?
>
> Linus's current tree would be fine as would:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-2.6.git
Ok I see, thanks. The patch should apply just fine to your tree, there
is only a spelling change since 2.6.38 which does not conflict.
Micha
^ permalink raw reply
* [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 7:01 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]
During the sctp_close() call, we do not use rcu primitives to
destroy the address list attached to the endpoint. At the same
time, we do the removal of addresses from this list before
attempting to remove the socket from the port hash
As a result, it is possible for another process to find the socket
in the port hash that is in the process of being closed. It then
proceeds to traverse the address list to find the conflict, only
to have that address list suddenly disappear without rcu() critical
section.
This can result in a kernel crash with general protection fault or
kernel NULL pointer dereference.
Fix issue by closing address list removal inside RCU critical
section.
Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
---
bind_addr.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index faf71d1..19d1329 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -155,8 +155,16 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
/* Dispose of an SCTP_bind_addr structure */
void sctp_bind_addr_free(struct sctp_bind_addr *bp)
{
- /* Empty the bind address list. */
- sctp_bind_addr_clean(bp);
+ struct sctp_sockaddr_entry *addr;
+
+ /* Empty the bind address list inside RCU section. */
+ rcu_read_lock();
+ list_for_each_entry_rcu(addr, &bp->address_list, list) {
+ list_del_rcu(&addr->list);
+ call_rcu(&addr->rcu, sctp_local_addr_free);
+ SCTP_DBG_OBJCNT_DEC(addr);
+ }
+ rcu_read_unlock();
if (bp->malloced) {
kfree(bp);
[-- Attachment #2: sctp_fix_close_vs_conflict_race_in_bind_addr.patch --]
[-- Type: application/octet-stream, Size: 732 bytes --]
diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
index faf71d1..19d1329 100644
--- a/net/sctp/bind_addr.c
+++ b/net/sctp/bind_addr.c
@@ -155,8 +155,16 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
/* Dispose of an SCTP_bind_addr structure */
void sctp_bind_addr_free(struct sctp_bind_addr *bp)
{
- /* Empty the bind address list. */
- sctp_bind_addr_clean(bp);
+ struct sctp_sockaddr_entry *addr;
+
+ /* Empty the bind address list inside RCU section. */
+ rcu_read_lock();
+ list_for_each_entry_rcu(addr, &bp->address_list, list) {
+ list_del_rcu(&addr->list);
+ call_rcu(&addr->rcu, sctp_local_addr_free);
+ SCTP_DBG_OBJCNT_DEC(addr);
+ }
+ rcu_read_unlock();
if (bp->malloced) {
kfree(bp);
^ permalink raw reply related
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Eric Dumazet @ 2011-05-18 7:48 UTC (permalink / raw)
To: Jacek Luczak; +Cc: netdev, Vlad Yasevich
In-Reply-To: <BANLkTikeWzuE-384uT6RhZR6Wn=DBK+CNQ@mail.gmail.com>
Le mercredi 18 mai 2011 à 09:01 +0200, Jacek Luczak a écrit :
> During the sctp_close() call, we do not use rcu primitives to
> destroy the address list attached to the endpoint. At the same
> time, we do the removal of addresses from this list before
> attempting to remove the socket from the port hash
>
> As a result, it is possible for another process to find the socket
> in the port hash that is in the process of being closed. It then
> proceeds to traverse the address list to find the conflict, only
> to have that address list suddenly disappear without rcu() critical
> section.
>
> This can result in a kernel crash with general protection fault or
> kernel NULL pointer dereference.
>
> Fix issue by closing address list removal inside RCU critical
> section.
>
> Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>
> ---
> bind_addr.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
> index faf71d1..19d1329 100644
> --- a/net/sctp/bind_addr.c
> +++ b/net/sctp/bind_addr.c
> @@ -155,8 +155,16 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
> /* Dispose of an SCTP_bind_addr structure */
> void sctp_bind_addr_free(struct sctp_bind_addr *bp)
> {
> - /* Empty the bind address list. */
> - sctp_bind_addr_clean(bp);
> + struct sctp_sockaddr_entry *addr;
> +
> + /* Empty the bind address list inside RCU section. */
> + rcu_read_lock();
> + list_for_each_entry_rcu(addr, &bp->address_list, list) {
> + list_del_rcu(&addr->list);
> + call_rcu(&addr->rcu, sctp_local_addr_free);
> + SCTP_DBG_OBJCNT_DEC(addr);
> + }
> + rcu_read_unlock();
>
Sorry this looks odd.
If you're removing items from this list, you must be a writer here, with
exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
Therefore, I guess following code is better :
list_for_each_entry(addr, &bp->address_list, list) {
list_del_rcu(&addr->list);
call_rcu(&addr->rcu, sctp_local_addr_free);
SCTP_DBG_OBJCNT_DEC(addr);
}
Then, why dont you fix sctp_bind_addr_clean() instead ?
if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
be protected as well.
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 8:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, Vlad Yasevich
In-Reply-To: <1305704885.2983.4.camel@edumazet-laptop>
2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
> Le mercredi 18 mai 2011 à 09:01 +0200, Jacek Luczak a écrit :
>> During the sctp_close() call, we do not use rcu primitives to
>> destroy the address list attached to the endpoint. At the same
>> time, we do the removal of addresses from this list before
>> attempting to remove the socket from the port hash
>>
>> As a result, it is possible for another process to find the socket
>> in the port hash that is in the process of being closed. It then
>> proceeds to traverse the address list to find the conflict, only
>> to have that address list suddenly disappear without rcu() critical
>> section.
>>
>> This can result in a kernel crash with general protection fault or
>> kernel NULL pointer dereference.
>>
>> Fix issue by closing address list removal inside RCU critical
>> section.
>>
>> Signed-off-by: Jacek Luczak <luczak.jacek@gmail.com>
>> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>>
>> ---
>> bind_addr.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/sctp/bind_addr.c b/net/sctp/bind_addr.c
>> index faf71d1..19d1329 100644
>> --- a/net/sctp/bind_addr.c
>> +++ b/net/sctp/bind_addr.c
>> @@ -155,8 +155,16 @@ static void sctp_bind_addr_clean(struct sctp_bind_addr *bp)
>> /* Dispose of an SCTP_bind_addr structure */
>> void sctp_bind_addr_free(struct sctp_bind_addr *bp)
>> {
>> - /* Empty the bind address list. */
>> - sctp_bind_addr_clean(bp);
>> + struct sctp_sockaddr_entry *addr;
>> +
>> + /* Empty the bind address list inside RCU section. */
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(addr, &bp->address_list, list) {
>> + list_del_rcu(&addr->list);
>> + call_rcu(&addr->rcu, sctp_local_addr_free);
>> + SCTP_DBG_OBJCNT_DEC(addr);
>> + }
>> + rcu_read_unlock();
>>
>
> Sorry this looks odd.
>
> If you're removing items from this list, you must be a writer here, with
> exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
I could agree to some extend ... but strict RCU section IMO is needed here.
I can check this if the issue exists.
> Therefore, I guess following code is better :
>
> list_for_each_entry(addr, &bp->address_list, list) {
> list_del_rcu(&addr->list);
> call_rcu(&addr->rcu, sctp_local_addr_free);
> SCTP_DBG_OBJCNT_DEC(addr);
> }
>
> Then, why dont you fix sctp_bind_addr_clean() instead ?
>
> if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
> be protected as well.
The _clean() as claimed by Vlad is called many times from various places
in code and this could give a overhead. I guess Vlad would need to comment.
-Jacek
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Eric Dumazet @ 2011-05-18 8:29 UTC (permalink / raw)
To: Jacek Luczak; +Cc: netdev, Vlad Yasevich
In-Reply-To: <BANLkTik75iqfgvWGKLJYu6E7mheOwOZX+A@mail.gmail.com>
Le mercredi 18 mai 2011 à 10:06 +0200, Jacek Luczak a écrit :
> 2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
> > If you're removing items from this list, you must be a writer here, with
> > exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
>
> I could agree to some extend ... but strict RCU section IMO is needed here.
> I can check this if the issue exists.
>
I can tell you for sure rcu_read_lock() is not needed here. Its only
showing confusion from code's author.
Please read Documentation/RCU/listRCU.txt for concise explanations,
line 117.
> > Therefore, I guess following code is better :
> >
> > list_for_each_entry(addr, &bp->address_list, list) {
> > list_del_rcu(&addr->list);
> > call_rcu(&addr->rcu, sctp_local_addr_free);
> > SCTP_DBG_OBJCNT_DEC(addr);
> > }
> >
> > Then, why dont you fix sctp_bind_addr_clean() instead ?
> >
> > if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
> > be protected as well.
>
> The _clean() as claimed by Vlad is called many times from various places
> in code and this could give a overhead. I guess Vlad would need to comment.
I guess a full review of this code is needed. You'll have to prove
sctp_bind_addr_clean() is always called after one RCU grace period if
you want to leave it as is.
You cant get RCU for free, some rules must be followed or you risk
crashes.
^ permalink raw reply
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Michael S. Tsirkin @ 2011-05-18 8:32 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <1305695674.10756.93.camel@localhost.localdomain>
On Tue, May 17, 2011 at 10:14:34PM -0700, Shirley Ma wrote:
> On Wed, 2011-05-18 at 00:28 +0300, Michael S. Tsirkin wrote:
> > On Tue, May 17, 2011 at 01:50:19PM -0700, Shirley Ma wrote:
> > > Resubmit the patch with most update. This patch passed some
> > > live-migration test against RHEL6.2. I will run more stress test w/i
> > > live migration.
> > >
> > > Signed-off-by: Shirley Ma <xma@us.ibm.com>
> >
> > Cool. cleanup path needs a fix - are you use you can
> > not use kobj or some other existing refcounting?
> I will look at this.
>
> > Is perf regressiion caused by tx ring overrun gone now?
> Nope.
>
> > I added some comments about how we might be aqble
> > to complete requests out of order but it's not a must.
> Ok.
>
> > > ---
> > >
> > > drivers/vhost/net.c | 37 +++++++++++++++++++++++++++++++-
> > > drivers/vhost/vhost.c | 55
> > ++++++++++++++++++++++++++++++++++++++++++++++++-
> > > drivers/vhost/vhost.h | 12 ++++++++++
> > > 3 files changed, 101 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 2f7c76a..6bd6e28 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -32,6 +32,9 @@
> > > * Using this limit prevents one virtqueue from starving others. */
> > > #define VHOST_NET_WEIGHT 0x80000
> > >
> > > +/* MAX number of TX used buffers for outstanding zerocopy */
> > > +#define VHOST_MAX_ZEROCOPY_PEND 128
> > > +
> > > enum {
> > > VHOST_NET_VQ_RX = 0,
> > > VHOST_NET_VQ_TX = 1,
> > > @@ -129,6 +132,7 @@ static void handle_tx(struct vhost_net *net)
> > > int err, wmem;
> > > size_t hdr_size;
> > > struct socket *sock;
> > > + struct skb_ubuf_info pend;
> > >
> > > /* TODO: check that we are running from vhost_worker? */
> > > sock = rcu_dereference_check(vq->private_data, 1);
> > > @@ -151,6 +155,10 @@ static void handle_tx(struct vhost_net *net)
> > > hdr_size = vq->vhost_hlen;
> > >
> > > for (;;) {
> > > + /* Release DMAs done buffers first */
> > > + if (atomic_read(&vq->refcnt) >
> > VHOST_MAX_ZEROCOPY_PEND)
> > > + vhost_zerocopy_signal_used(vq, false);
> > > +
> > > head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> > > ARRAY_SIZE(vq->iov),
> > > &out, &in,
> > > @@ -166,6 +174,13 @@ static void handle_tx(struct vhost_net *net)
> > > set_bit(SOCK_ASYNC_NOSPACE,
> > &sock->flags);
> > > break;
> > > }
> > > + /* If more outstanding DMAs, queue the work */
> > > + if (sock_flag(sock->sk, SOCK_ZEROCOPY) &&
> > > + (atomic_read(&vq->refcnt) >
> > VHOST_MAX_ZEROCOPY_PEND)) {
> > > + tx_poll_start(net, sock);
> > > + set_bit(SOCK_ASYNC_NOSPACE,
> > &sock->flags);
> > > + break;
> > > + }
> > > if (unlikely(vhost_enable_notify(vq))) {
> > > vhost_disable_notify(vq);
> > > continue;
> > > @@ -188,17 +203,35 @@ static void handle_tx(struct vhost_net *net)
> > > iov_length(vq->hdr, s), hdr_size);
> > > break;
> > > }
> > > + /* use msg_control to pass vhost zerocopy ubuf info to
> > skb */
> > > + if (sock_flag(sock->sk, SOCK_ZEROCOPY)) {
> > > + vq->heads[vq->upend_idx].id = head;
> > > + if (len <= 128)
> >
> > I thought we have a constant for that?
>
> Yes, fixed it already. I might change it to PAGE_SIZE for now since the
> small message sizes regression issue.
>
> > > + vq->heads[vq->upend_idx].len =
> > VHOST_DMA_DONE_LEN;
> > > + else {
> > > + vq->heads[vq->upend_idx].len = len;
> > > + pend.callback =
> > vhost_zerocopy_callback;
> > > + pend.arg = vq;
> > > + pend.desc = vq->upend_idx;
> > > + msg.msg_control = &pend;
> > > + msg.msg_controllen = sizeof(pend);
> > > + }
> > > + atomic_inc(&vq->refcnt);
> > > + vq->upend_idx = (vq->upend_idx + 1) %
> > UIO_MAXIOV;
> >
> > Ok, so we deal with a cyclic ring apparently? What guarantees we don't
> > overrun it?
>
> VHOST_MAX_PEND (which is 128) should prevent it from overrun normally.
> In the case of none DMAs can be done, the maximum entries are the ring
> size, which is much smaller or equal to UIO_MAXIOV for current
> implementation.
>
> Maybe I should add some condition check if it is overrun then exits?
If this can't happen, maybe add a comment why. A check + WARN_ON
can't hurt either.
> >
> > > + }
> > > /* TODO: Check specific error and bomb out unless
> > ENOBUFS? */
> > > err = sock->ops->sendmsg(NULL, sock, &msg, len);
> > > if (unlikely(err < 0)) {
> > > - vhost_discard_vq_desc(vq, 1);
> > > + if (!sock_flag(sock->sk, SOCK_ZEROCOPY))
> > > + vhost_discard_vq_desc(vq, 1);
> >
> > How are errors handled with zerocopy?
>
> I thought about it before: if error after macvtap skb is allocated, then
> that skb has a callback which will set DMA done for add_used, if the
> error before macvtap skb is allocated, then it can reverse as copy case.
> To avoid the complexity check, I decided to not handle it.
I think we need to handle it, errors do happen.
macvtap should either don't invoke callback on error or return success.
Then it's simple.
> >
> > > tx_poll_start(net, sock);
> > > break;
> > > }
> > > if (err != len)
> > > pr_debug("Truncated TX packet: "
> > > " len %d != %zd\n", err, len);
> > > - vhost_add_used_and_signal(&net->dev, vq, head, 0);
> > > + if (!sock_flag(sock->sk, SOCK_ZEROCOPY))
> > > + vhost_add_used_and_signal(&net->dev, vq, head,
> > 0);
> > > total_len += len;
> > > if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> > > vhost_poll_queue(&vq->poll);
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 2ab2912..ce799d6 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -174,6 +174,9 @@ static void vhost_vq_reset(struct vhost_dev
> > *dev,
> > > vq->call_ctx = NULL;
> > > vq->call = NULL;
> > > vq->log_ctx = NULL;
> > > + vq->upend_idx = 0;
> > > + vq->done_idx = 0;
> > > + atomic_set(&vq->refcnt, 0);
> > > }
> > >
> > > static int vhost_worker(void *data)
> > > @@ -230,7 +233,7 @@ static long vhost_dev_alloc_iovecs(struct
> > vhost_dev *dev)
> > > UIO_MAXIOV,
> > GFP_KERNEL);
> > > dev->vqs[i].log = kmalloc(sizeof *dev->vqs[i].log *
> > UIO_MAXIOV,
> > > GFP_KERNEL);
> > > - dev->vqs[i].heads = kmalloc(sizeof *dev->vqs[i].heads
> > *
> > > + dev->vqs[i].heads = kzalloc(sizeof *dev->vqs[i].heads
> > *
> > > UIO_MAXIOV, GFP_KERNEL);
> >
> > Which fields need to be initialized actually?
>
> Nope, already fixed it with kmalloc.
>
> > >
> > > if (!dev->vqs[i].indirect || !dev->vqs[i].log ||
> > > @@ -385,6 +388,38 @@ long vhost_dev_reset_owner(struct vhost_dev
> > *dev)
> > > return 0;
> > > }
> > >
> > > +/*
> > > + comments
> > > +*/
> >
> > Hmm.
>
> Fixed it in previous patch already.
>
> > > +void vhost_zerocopy_signal_used(struct vhost_virtqueue *vq, bool
> > shutdown)
> > > +{
> > > + int i, j = 0;
> > > +
> > > + i = vq->done_idx;
> > > + while (i != vq->upend_idx) {
> >
> > A for loop might be clearer.
> Ok.
>
> > > + if ((vq->heads[i].len == VHOST_DMA_DONE_LEN) ||
> > shutdown) {
> >
> > On shutdown, we signal all buffers used to the guest?
> > Why?
>
> We signal all outstand DMAs in the case of driver has some DMA issue to
> prevent us from shutting down. I am afraid vhost cleanup could wait
> forever.
Yes but then
1. guest can reuse the buffers for something else, before device reads them
2. vhost-net module can go away
etc
IMO that this happens in finite time is one of the things driver should
guarantee when it sets the zcopy flags.
BTW we must flush these on memory table change too, right?
> > > + /* reset len = 0 */
> >
> > comment not very helpful.
> > Could you explain what this does instead?
> > Or better use some constant instead of 0 ...
>
> Fixed it already.
>
> > > + vq->heads[i].len = 0;
> > > + i = (i + 1) % UIO_MAXIOV;
> > > + ++j;
> > > + } else
> > > + break;
> >
> > Hmm so if the 1st entry does not complete, you do not signal anything?
>
> No used buffers to guest, should we signal still?
This is just myself noting that we still force used entries
to be in order. It's ok from correctness pov but likely
is at least part of the reason you still see
TX ring overruns.
> > > + }
> >
> > Looking at this loop, done idx is the consumer and used idx
> > is the producer, right?
>
> Yes.
>
> > > + if (j) {
> > > + /* comments */
> >
> > Yes?
> >
> > > + if (i > vq->done_idx)
> > > + vhost_add_used_n(vq, &vq->heads[vq->done_idx],
> > j);
> > > + else {
> > > + vhost_add_used_n(vq, &vq->heads[vq->done_idx],
> > > + UIO_MAXIOV - vq->done_idx);
> > > + vhost_add_used_n(vq, vq->heads, i);
> > > + }
> > > + vq->done_idx = i;
> > > + vhost_signal(vq->dev, vq);
> > > + atomic_sub(j, &vq->refcnt);
> >
> > Code will likely be simpler if you call vhost_add_used once for
> > each head in the first loop. Possibly add_used_signal might be
> > a good idea too.
>
> Ok.
>
> > > + }
> > > +}
> > > +
> > > /* Caller should have device mutex */
> > > void vhost_dev_cleanup(struct vhost_dev *dev)
> > > {
> > > @@ -395,6 +430,11 @@ void vhost_dev_cleanup(struct vhost_dev *dev)
> > > vhost_poll_stop(&dev->vqs[i].poll);
> > > vhost_poll_flush(&dev->vqs[i].poll);
> > > }
> > > + /* wait for all lower device DMAs done, then notify
> > guest */
> > > + if (atomic_read(&dev->vqs[i].refcnt)) {
> > > + msleep(1000);
> > > + vhost_zerocopy_signal_used(&dev->vqs[i],
> > true);
> > > + }
> >
> > This needs to be fixed somehow. Use a completion object and wait
> > on it?
>
> Worried about what if the driver has some DMAs issue, which would
> prevent vhost from shutting down.
This needs to be fixed in the driver then.
> > > if (dev->vqs[i].error_ctx)
> > > eventfd_ctx_put(dev->vqs[i].error_ctx);
> > > if (dev->vqs[i].error)
> > > @@ -603,6 +643,10 @@ static long vhost_set_vring(struct vhost_dev
> > *d, int ioctl, void __user *argp)
> > >
> > > mutex_lock(&vq->mutex);
> > >
> > > + /* force all lower device DMAs done */
> > > + if (atomic_read(&vq->refcnt))
> > > + vhost_zerocopy_signal_used(vq, true);
> > > +
> > > switch (ioctl) {
> > > case VHOST_SET_VRING_NUM:
> > > /* Resizing ring with an active backend?
> > > @@ -1416,3 +1460,12 @@ void vhost_disable_notify(struct
> > vhost_virtqueue *vq)
> > > vq_err(vq, "Failed to enable notification at %p: %d
> > \n",
> > > &vq->used->flags, r);
> > > }
> > > +
> > > +void vhost_zerocopy_callback(struct sk_buff *skb)
> > > +{
> > > + int idx = skb_shinfo(skb)->ubuf.desc;
> > > + struct vhost_virtqueue *vq = skb_shinfo(skb)->ubuf.arg;
> > > +
> > > + /* set len = 1 to mark this desc buffers done DMA */
> >
> > this comment can now go.
>
> Yes, it's gone already.
>
> > > + vq->heads[idx].len = VHOST_DMA_DONE_LEN;
> > > +}
> > > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > > index b3363ae..8e3ecc7 100644
> > > --- a/drivers/vhost/vhost.h
> > > +++ b/drivers/vhost/vhost.h
> > > @@ -13,6 +13,10 @@
> > > #include <linux/virtio_ring.h>
> > > #include <asm/atomic.h>
> > >
> > > +/* This is for zerocopy, used buffer len is set to 1 when lower
> > device DMA
> > > + * done */
> > > +#define VHOST_DMA_DONE_LEN 1
> > > +
> > > struct vhost_device;
> > >
> > > struct vhost_work;
> > > @@ -108,6 +112,12 @@ struct vhost_virtqueue {
> > > /* Log write descriptors */
> > > void __user *log_base;
> > > struct vhost_log *log;
> > > + /* vhost zerocopy support */
> > > + atomic_t refcnt; /* num of outstanding zerocopy DMAs */
> >
> > future enhancement idea: this is used apparently under vq lock
> > so no need for an atomic?
>
> It is also used in skb vhost zerocopy callback.
>
> > > + /* copy of avail idx to monitor outstanding DMA zerocopy
> > buffers */
> >
> > looking at code upend_idx seems to be calculated independently
> > of guest avail idx - could you clarify pls?
>
> Yes, you are right. Should change it to: upend_idx is used to track
> vring ids for outstanding zero-copy DMA buffers?
mention producer-consumer index in a head array used as
a circular ring datastructure.
> > > + int upend_idx;
> > > + /* copy of used idx to monintor DMA done zerocopy buffers */
> >
> > monitor
>
> Ok.
>
> > > + int done_idx;
> >
> >
> > I think in reality these are just producer and consumer
> > in the head structure which for zero copy is used
> Yes.
>
> >
> > > };
> > >
> > > struct vhost_dev {
> > > @@ -154,6 +164,8 @@ bool vhost_enable_notify(struct vhost_virtqueue
> > *);
> > >
> > > int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log
> > *log,
> > > unsigned int log_num, u64 len);
> > > +void vhost_zerocopy_callback(struct sk_buff *skb);
> > > +void vhost_zerocopy_signal_used(struct vhost_virtqueue *vq, bool
> > shutdown);
> > >
> > > #define vq_err(vq, fmt, ...) do {
> > \
> > > pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
> > >
^ permalink raw reply
* Re: [PATCH V6 4/6 net-next] vhost: vhost TX zero-copy support
From: Michael S. Tsirkin @ 2011-05-18 8:43 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <1305699383.10756.100.camel@localhost.localdomain>
On Tue, May 17, 2011 at 11:16:23PM -0700, Shirley Ma wrote:
> Hello Michael,
>
> Here is the update the patch based on all of your review comments except
> the completion/wait for cleanup since I am worried about outstanding
> DMAs would prevent vhost from shutting down.
Don't see what you are worried about. Doing this in a timely fashion
is just one of the things driver commits to when it published the zcopy flag.
Otherwise guest networking will get stuck as entries in the tx ring
don't free up, which is also a problem. Let's just add a comment documenting this.
> I am sending out this for your review, and test it out later.
So let's use a completion to cleanly flush outstanding requests.
Any chance kref can be reused? It's not a must.
One other piece that is currently missing is flushing
requests on memory table update. Maybe we can stick some
info in high bits of the desc field for that?
> For error handling, I update macvtap.c so we can discard the desc even
> in zero-copy case.
>
> Signed-off-by: Shirley Ma <xma@us.ibm.com>
> ---
> drivers/vhost/net.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> drivers/vhost/vhost.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/vhost/vhost.h | 13 +++++++++++++
> 3 files changed, 103 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 2f7c76a..e87a1f8 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -32,6 +32,10 @@
> * Using this limit prevents one virtqueue from starving others. */
> #define VHOST_NET_WEIGHT 0x80000
>
> +/* MAX number of TX used buffers for outstanding zerocopy */
> +#define VHOST_MAX_PEND 128
> +#define VHOST_GOODCOPY_LEN PAGE_SIZE
> +
> enum {
> VHOST_NET_VQ_RX = 0,
> VHOST_NET_VQ_TX = 1,
> @@ -129,6 +133,7 @@ static void handle_tx(struct vhost_net *net)
> int err, wmem;
> size_t hdr_size;
> struct socket *sock;
> + struct skb_ubuf_info pend;
>
> /* TODO: check that we are running from vhost_worker? */
> sock = rcu_dereference_check(vq->private_data, 1);
> @@ -151,6 +156,10 @@ static void handle_tx(struct vhost_net *net)
> hdr_size = vq->vhost_hlen;
>
> for (;;) {
> + /* Release DMAs done buffers first */
> + if (atomic_read(&vq->refcnt) > VHOST_MAX_PEND)
> + vhost_zerocopy_signal_used(vq, false);
> +
> head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
> ARRAY_SIZE(vq->iov),
> &out, &in,
> @@ -166,6 +175,12 @@ static void handle_tx(struct vhost_net *net)
> set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> break;
> }
> + /* If more outstanding DMAs, queue the work */
> + if (atomic_read(&vq->refcnt) > VHOST_MAX_PEND) {
> + tx_poll_start(net, sock);
> + set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> + break;
> + }
> if (unlikely(vhost_enable_notify(vq))) {
> vhost_disable_notify(vq);
> continue;
> @@ -188,6 +203,30 @@ static void handle_tx(struct vhost_net *net)
> iov_length(vq->hdr, s), hdr_size);
> break;
> }
> + /* use msg_control to pass vhost zerocopy ubuf info to skb */
> + if (sock_flag(sock->sk, SOCK_ZEROCOPY)) {
> + vq->heads[vq->upend_idx].id = head;
> + if (len < VHOST_GOODCOPY_LEN)
> + /* copy don't need to wait for DMA done */
> + vq->heads[vq->upend_idx].len =
> + VHOST_DMA_DONE_LEN;
> + else {
> + vq->heads[vq->upend_idx].len = len;
> + pend.callback = vhost_zerocopy_callback;
> + pend.arg = vq;
> + pend.desc = vq->upend_idx;
> + msg.msg_control = &pend;
> + msg.msg_controllen = sizeof(pend);
> + }
> + atomic_inc(&vq->refcnt);
> + vq->upend_idx = (vq->upend_idx + 1) % UIO_MAXIOV;
> + /* if upend_idx is full, then wait for free more */
> + if (vq->upend_idx == vq->done_idx) {
> + tx_poll_start(net, sock);
> + set_bit(SOCK_ASYNC_NOSPACE, &sock->flags);
> + break;
> + }
> + }
> /* TODO: Check specific error and bomb out unless ENOBUFS? */
> err = sock->ops->sendmsg(NULL, sock, &msg, len);
> if (unlikely(err < 0)) {
> @@ -198,7 +237,8 @@ static void handle_tx(struct vhost_net *net)
> if (err != len)
> pr_debug("Truncated TX packet: "
> " len %d != %zd\n", err, len);
> - vhost_add_used_and_signal(&net->dev, vq, head, 0);
> + if (!sock_flag(sock->sk, SOCK_ZEROCOPY))
> + vhost_add_used_and_signal(&net->dev, vq, head, 0);
> total_len += len;
> if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
> vhost_poll_queue(&vq->poll);
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 2ab2912..f4c2730 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -174,6 +174,9 @@ static void vhost_vq_reset(struct vhost_dev *dev,
> vq->call_ctx = NULL;
> vq->call = NULL;
> vq->log_ctx = NULL;
> + vq->upend_idx = 0;
> + vq->done_idx = 0;
> + atomic_set(&vq->refcnt, 0);
> }
>
> static int vhost_worker(void *data)
> @@ -385,16 +388,49 @@ long vhost_dev_reset_owner(struct vhost_dev *dev)
> return 0;
> }
>
> +/* In case of DMA done not in order in lower device driver for some reason.
> + * upend_idx is used to track end of used idx, done_idx is used to track head
> + * of used idx. Once lower device DMA done contiguously, we will signal KVM
> + * guest used idx.
> + */
> +void vhost_zerocopy_signal_used(struct vhost_virtqueue *vq, bool shutdown)
> +{
> + int i, j = 0;
> +
> + for (i = vq->done_idx; i != vq->upend_idx; i = i++ % UIO_MAXIOV) {
> + if ((vq->heads[i].len == VHOST_DMA_DONE_LEN) || shutdown) {
> + vq->heads[i].len = VHOST_DMA_CLEAR_LEN;
> + vhost_add_used_and_signal(vq->dev, vq,
> + vq->heads[i].id, 0);
> + ++j;
> + } else
> + break;
> + }
> + if (j) {
> + vq->done_idx = i;
> + atomic_sub(j, &vq->refcnt);
> + }
> +}
> +
> /* Caller should have device mutex */
> void vhost_dev_cleanup(struct vhost_dev *dev)
> {
> int i;
> + unsigned long begin = jiffies;
>
> for (i = 0; i < dev->nvqs; ++i) {
> if (dev->vqs[i].kick && dev->vqs[i].handle_kick) {
> vhost_poll_stop(&dev->vqs[i].poll);
> vhost_poll_flush(&dev->vqs[i].poll);
> }
> + /* Wait for all lower device DMAs done, then notify virtio_net
> + * or just notify it without waiting for all DMA done here ?
> + * in case of DMAs never done for some reason */
> + if (atomic_read(&dev->vqs[i].refcnt)) {
> + /* how long should we wait ? */
> + msleep(1000);
> + vhost_zerocopy_signal_used(&dev->vqs[i], true);
> + }
> if (dev->vqs[i].error_ctx)
> eventfd_ctx_put(dev->vqs[i].error_ctx);
> if (dev->vqs[i].error)
> @@ -603,6 +639,10 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
>
> mutex_lock(&vq->mutex);
>
> + /* clean up lower device outstanding DMAs, before setting ring */
> + if (atomic_read(&vq->refcnt))
> + vhost_zerocopy_signal_used(vq, true);
> +
> switch (ioctl) {
> case VHOST_SET_VRING_NUM:
> /* Resizing ring with an active backend?
> @@ -1416,3 +1456,12 @@ void vhost_disable_notify(struct vhost_virtqueue *vq)
> vq_err(vq, "Failed to enable notification at %p: %d\n",
> &vq->used->flags, r);
> }
> +
> +void vhost_zerocopy_callback(struct sk_buff *skb)
> +{
> + int idx = skb_shinfo(skb)->ubuf.desc;
> + struct vhost_virtqueue *vq = skb_shinfo(skb)->ubuf.arg;
Let's do some sanity checking of idx here in case
there's a driver bug.
> +
> + /* set len = 1 to mark this desc buffers done DMA */
> + vq->heads[idx].len = VHOST_DMA_DONE_LEN;
> +}
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index b3363ae..d0e7ac6 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -13,6 +13,11 @@
> #include <linux/virtio_ring.h>
> #include <asm/atomic.h>
>
> +/* This is for zerocopy, used buffer len is set to 1 when lower device DMA
> + * done */
> +#define VHOST_DMA_DONE_LEN 1
> +#define VHOST_DMA_CLEAR_LEN 0
> +
> struct vhost_device;
>
> struct vhost_work;
> @@ -108,6 +113,12 @@ struct vhost_virtqueue {
> /* Log write descriptors */
> void __user *log_base;
> struct vhost_log *log;
> + /* vhost zerocopy support */
> + atomic_t refcnt; /* num of outstanding zerocopy DMAs */
> + /* last used idx for outstanding DMA zerocopy buffers */
> + int upend_idx;
> + /* first used idx for DMA done zerocopy buffers */
> + int done_idx;
> };
>
> struct vhost_dev {
> @@ -154,6 +165,8 @@ bool vhost_enable_notify(struct vhost_virtqueue *);
>
> int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
> unsigned int log_num, u64 len);
> +void vhost_zerocopy_callback(struct sk_buff *skb);
> +void vhost_zerocopy_signal_used(struct vhost_virtqueue *vq, bool shutdown);
>
> #define vq_err(vq, fmt, ...) do { \
> pr_debug(pr_fmt(fmt), ##__VA_ARGS__); \
>
>
^ permalink raw reply
* Re: [PATCH V5 4/6 net-next] vhost: vhost TX zero-copy support
From: Michael S. Tsirkin @ 2011-05-18 8:45 UTC (permalink / raw)
To: Shirley Ma
Cc: David Miller, Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev,
kvm, linux-kernel
In-Reply-To: <1305695674.10756.93.camel@localhost.localdomain>
On Tue, May 17, 2011 at 10:14:34PM -0700, Shirley Ma wrote:
> Yes, fixed it already. I might change it to PAGE_SIZE for now since the
> small message sizes regression issue.
If that fixes it, let's do that for now.
--
MST
^ permalink raw reply
* packet received in a wrong rx-queue?
From: Jon Zhou @ 2011-05-18 9:00 UTC (permalink / raw)
To: e1000-devel@lists.sourceforge.net, netdev@vger.kernel.org
There are 2 packets in the traffic
#1 create PDP context request, IPV4--UDP--GTP, src_ip=A and dst_ip=B,src_port=C,dst_port=D
#2 create PDP context response, IPV4--UDP--GTP,src_ip=B, dst_ip=A, src_port=D,dst_port=C
I suppose both of them will be received in same rx-queue but actually it doesn't
Anything need to check?
ethtool -i eth5
driver: ixgbe
version: 3.3.9-NAPI
firmware-version: 0.9-3
bus-info: 0000:08:00.1
regards
jon
------------------------------------------------------------------------------
What Every C/C++ and Fortran developer Should Know!
Read this article and learn how Intel has extended the reach of its
next-generation tools to help Windows* and Linux* C/C++ and Fortran
developers boost performance applications - including clusters.
http://p.sf.net/sfu/intel-dev2devmay
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Wei Yongjun @ 2011-05-18 9:02 UTC (permalink / raw)
To: Jacek Luczak; +Cc: Eric Dumazet, netdev, Vlad Yasevich
In-Reply-To: <1305707358.2983.14.camel@edumazet-laptop>
> Le mercredi 18 mai 2011 à 10:06 +0200, Jacek Luczak a écrit :
>> 2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
>>> If you're removing items from this list, you must be a writer here, with
>>> exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
>> I could agree to some extend ... but strict RCU section IMO is needed here.
>> I can check this if the issue exists.
>>
> I can tell you for sure rcu_read_lock() is not needed here. Its only
> showing confusion from code's author.
>
> Please read Documentation/RCU/listRCU.txt for concise explanations,
> line 117.
>
>
>>> Therefore, I guess following code is better :
>>>
>>> list_for_each_entry(addr, &bp->address_list, list) {
>>> list_del_rcu(&addr->list);
>>> call_rcu(&addr->rcu, sctp_local_addr_free);
>>> SCTP_DBG_OBJCNT_DEC(addr);
>>> }
>>>
>>> Then, why dont you fix sctp_bind_addr_clean() instead ?
>>>
>>> if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
>>> be protected as well.
>> The _clean() as claimed by Vlad is called many times from various places
>> in code and this could give a overhead. I guess Vlad would need to comment.
> I guess a full review of this code is needed. You'll have to prove
> sctp_bind_addr_clean() is always called after one RCU grace period if
> you want to leave it as is.
>
> You cant get RCU for free, some rules must be followed or you risk
> crashes.
>
fix the race between sctp_bind_addr_free() and sctp_bind_addr_conflict(), maybe you just
need to remove the socket from the port hash before empty the bind address list.
some thing like this, not test.
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index c8cc24e..924d846 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -268,12 +268,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
/* Cleanup. */
sctp_inq_free(&ep->base.inqueue);
- sctp_bind_addr_free(&ep->base.bind_addr);
/* Remove and free the port */
if (sctp_sk(ep->base.sk)->bind_hash)
sctp_put_port(ep->base.sk);
+ sctp_bind_addr_free(&ep->base.bind_addr);
+
/* Give up our hold on the sock. */
if (ep->base.sk)
sock_put(ep->base.sk);
^ permalink raw reply related
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michał Mirosław @ 2011-05-18 9:06 UTC (permalink / raw)
To: Shirley Ma
Cc: Michael S. Tsirkin, Ben Hutchings, David Miller, Eric Dumazet,
Avi Kivity, Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <1305675865.10756.63.camel@localhost.localdomain>
W dniu 18 maja 2011 01:44 użytkownik Shirley Ma <mashirle@us.ibm.com> napisał:
> On Wed, 2011-05-18 at 00:58 +0200, Michał Mirosław wrote:
>> W dniu 18 maja 2011 00:28 użytkownik Shirley Ma <mashirle@us.ibm.com>
>> napisał:
>> > On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> >> 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> >> > Looks like to use a new flag requires more time/work. I am
>> thinking
>> >> > whether we can just use HIGHDMA flag to enable zero-copy in
>> macvtap
>> >> to
>> >> > avoid the new flag for now since mavctap uses real NICs as lower
>> >> device?
>> >>
>> >> Is there any other restriction besides requiring driver to not
>> recycle
>> >> the skb? Are there any drivers that recycle TX skbs?
>> > Not more other restrictions, skb clone is OK. pskb_expand_head()
>> looks
>> > OK to me from code review.
>>
>> > Currently there is no drivers recycle TX skbs.
>>
>> So why do you require the target device to have some flags at all?
> We could use macvtap to check lower device HIGHDMA to enable zero-copy,
> but I am not sure whether it is sufficient. If it's sufficient then we
> don't need to use a new flag here. To be safe, it's better to use a new
> flag to enable each device who can pass zero-copy test.
>> Do I understand correctly, that this zero-copy feature is about
>> packets received from VMs?
> Yes, packets sent from VMs, and received in local host for TX zero-copy
> here.
What is the zero-copy test? On some arches the HIGHDMA is not needed
at all so might be not enabled on anything. It looks like the correct
test would be per-packet check of !illegal_highdma() or maybe
NETIF_F_SG as returned from harmonize_features(). For virtual devices
or other software forwarding this might lead to skb_linearize() in
some cases, but is it that bad?
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: packet received in a wrong rx-queue?
From: David Miller @ 2011-05-18 9:13 UTC (permalink / raw)
To: Jon.Zhou; +Cc: e1000-devel, netdev
In-Reply-To: <4A6A2125329CFD4D8CC40C9E8ABCAB9F250D748939@MILEXCH2.ds.jdsu.net>
From: Jon Zhou <Jon.Zhou@jdsu.com>
Date: Wed, 18 May 2011 02:00:50 -0700
> There are 2 packets in the traffic
>
> #1 create PDP context request, IPV4--UDP--GTP, src_ip=A and dst_ip=B,src_port=C,dst_port=D
>
> #2 create PDP context response, IPV4--UDP--GTP,src_ip=B, dst_ip=A, src_port=D,dst_port=C
>
> I suppose both of them will be received in same rx-queue but actually it doesn't
Well, for hardware flow steering, it's not expected to.
^ permalink raw reply
* Re: Bug, kernel panic, NULL dereference , cleanup_once / icmp_route_lookup.clone.19.clone / nat , 2.6.39-rc7-git11
From: Denys Fedoryshchenko @ 2011-05-18 9:27 UTC (permalink / raw)
To: netdev
In-Reply-To: <54ec5cd14e5e5c76aa06c2e6899299ce@visp.net.lb>
On Wed, 18 May 2011 01:16:29 +0300, Denys Fedoryshchenko wrote:
> Just got recently. 32Bit, PPPoE NAS, shapers, firewall, NAT
> Kernel i mention in subject, 2.6.39-rc7-git11
> If required i can give more information
>
> sharanal (sorry for ugly name) is libpcap based traffic analyser,
> sure userspace
>
Here is some info, i hope it will be a little useful
(gdb) l *(cleanup_once + 0x49)
0xc02e85cc is in cleanup_once (include/linux/list.h:88).
83 * This is only for internal list manipulation where we know
84 * the prev/next entries already!
85 */
86 static inline void __list_del(struct list_head * prev, struct
list_head * next)
87 {
88 next->prev = prev;
89 prev->next = next;
90 }
91
92 /**
(gdb) l *(inet_getpeer + 0x2ab)
0xc02e8ae8 is in inet_getpeer (net/ipv4/inetpeer.c:530).
525 if (base->total >= inet_peer_threshold)
526 /* Remove one less-recently-used entry. */
527 cleanup_once(0, stack);
528
529 return p;
530 }
531
532 static int compute_total(void)
533 {
534 return v4_peers.total + v6_peers.total;
^ permalink raw reply
* [PATCH net-next-2.6] net: make sure rtnl is held in rtnl_fill_ifinfo()
From: Eric Dumazet @ 2011-05-18 9:35 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Commit e67f88dd12f610 (net: dont hold rtnl mutex during netlink dump
callbacks) added a problem in rtnl_fill_ifinfo() not being always called
with RTNL held, which is racy.
1) This patch extends rtnl_mutex helper functions so that :
rtnl_lock() is able to BUG_ON() if recursively called.
[This was only provided if LOCKDEP was on]
rtnl_is_locked() is able to check if current thread owns rtnl_mutex
[ASSERT_RTNL() gets this added feature too]
2) Add one ASSERT_RTNL() in rtnl_fill_ifinfo()
3) Make sure rtnl is held in rtnl_dump_ifinfo()
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
net/core/rtnetlink.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d2ba259..4e09f70 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -59,15 +59,19 @@ struct rtnl_link {
};
static DEFINE_MUTEX(rtnl_mutex);
+static struct thread_info *rtnl_owner;
void rtnl_lock(void)
{
+ BUG_ON(rtnl_owner == current_thread_info());
mutex_lock(&rtnl_mutex);
+ rtnl_owner = current_thread_info();
}
EXPORT_SYMBOL(rtnl_lock);
void __rtnl_unlock(void)
{
+ rtnl_owner = NULL;
mutex_unlock(&rtnl_mutex);
}
@@ -80,13 +84,17 @@ EXPORT_SYMBOL(rtnl_unlock);
int rtnl_trylock(void)
{
- return mutex_trylock(&rtnl_mutex);
+ int ret = mutex_trylock(&rtnl_mutex);
+
+ if (ret)
+ rtnl_owner = current_thread_info();
+ return ret;
}
EXPORT_SYMBOL(rtnl_trylock);
int rtnl_is_locked(void)
{
- return mutex_is_locked(&rtnl_mutex);
+ return mutex_is_locked(&rtnl_mutex) && rtnl_owner == current_thread_info();
}
EXPORT_SYMBOL(rtnl_is_locked);
@@ -850,6 +858,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
struct nlattr *attr, *af_spec;
struct rtnl_af_ops *af_ops;
+ ASSERT_RTNL();
nlh = nlmsg_put(skb, pid, seq, type, sizeof(*ifm), flags);
if (nlh == NULL)
return -EMSGSIZE;
@@ -1003,10 +1012,12 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
struct net_device *dev;
struct hlist_head *head;
struct hlist_node *node;
+ int rtnl_was_locked = rtnl_is_locked();
s_h = cb->args[0];
s_idx = cb->args[1];
-
+ if (!rtnl_was_locked)
+ rtnl_lock();
rcu_read_lock();
for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
idx = 0;
@@ -1025,6 +1036,8 @@ cont:
}
out:
rcu_read_unlock();
+ if (!rtnl_was_locked)
+ rtnl_unlock();
cb->args[1] = idx;
cb->args[0] = h;
^ permalink raw reply related
* Re: Bug, kernel panic, NULL dereference , cleanup_once / icmp_route_lookup.clone.19.clone / nat , 2.6.39-rc7-git11
From: Eric Dumazet @ 2011-05-18 9:37 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
In-Reply-To: <41a1892fed59b411bb08d3ecb0d8cda5@visp.net.lb>
Le mercredi 18 mai 2011 à 12:27 +0300, Denys Fedoryshchenko a écrit :
> On Wed, 18 May 2011 01:16:29 +0300, Denys Fedoryshchenko wrote:
> > Just got recently. 32Bit, PPPoE NAS, shapers, firewall, NAT
> > Kernel i mention in subject, 2.6.39-rc7-git11
> > If required i can give more information
> >
> > sharanal (sorry for ugly name) is libpcap based traffic analyser,
> > sure userspace
> >
> Here is some info, i hope it will be a little useful
>
> (gdb) l *(cleanup_once + 0x49)
> 0xc02e85cc is in cleanup_once (include/linux/list.h:88).
> 83 * This is only for internal list manipulation where we know
> 84 * the prev/next entries already!
> 85 */
> 86 static inline void __list_del(struct list_head * prev, struct
> list_head * next)
> 87 {
> 88 next->prev = prev;
> 89 prev->next = next;
> 90 }
> 91
> 92 /**
>
> (gdb) l *(inet_getpeer + 0x2ab)
> 0xc02e8ae8 is in inet_getpeer (net/ipv4/inetpeer.c:530).
> 525 if (base->total >= inet_peer_threshold)
> 526 /* Remove one less-recently-used entry. */
> 527 cleanup_once(0, stack);
> 528
> 529 return p;
> 530 }
> 531
> 532 static int compute_total(void)
> 533 {
> 534 return v4_peers.total + v6_peers.total;
>
I really begin to think we have a bug here...
In previous reports, I suggested to use slub_nomerge because I thought
one corruption from another kernel layer was going on.
(inetpeer was using 64 bytes objects). But now that inetpeer objects are
bigger and sit in another kmemcache, its bad news.
Could you try this, and eventually add some SLUB debugging stuff as
well ?
^ permalink raw reply
* Re: Bug, kernel panic, NULL dereference , cleanup_once / icmp_route_lookup.clone.19.clone / nat , 2.6.39-rc7-git11
From: Denys Fedoryshchenko @ 2011-05-18 9:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1305711471.2983.27.camel@edumazet-laptop>
On Wed, 18 May 2011 11:37:51 +0200, Eric Dumazet wrote:
> Le mercredi 18 mai 2011 à 12:27 +0300, Denys Fedoryshchenko a écrit :
>> On Wed, 18 May 2011 01:16:29 +0300, Denys Fedoryshchenko wrote:
>> > Just got recently. 32Bit, PPPoE NAS, shapers, firewall, NAT
>> > Kernel i mention in subject, 2.6.39-rc7-git11
>> > If required i can give more information
>> >
>> > sharanal (sorry for ugly name) is libpcap based traffic analyser,
>> > sure userspace
>> >
>> Here is some info, i hope it will be a little useful
>>
>> (gdb) l *(cleanup_once + 0x49)
>> 0xc02e85cc is in cleanup_once (include/linux/list.h:88).
>> 83 * This is only for internal list manipulation where we
>> know
>> 84 * the prev/next entries already!
>> 85 */
>> 86 static inline void __list_del(struct list_head * prev,
>> struct
>> list_head * next)
>> 87 {
>> 88 next->prev = prev;
>> 89 prev->next = next;
>> 90 }
>> 91
>> 92 /**
>>
>> (gdb) l *(inet_getpeer + 0x2ab)
>> 0xc02e8ae8 is in inet_getpeer (net/ipv4/inetpeer.c:530).
>> 525 if (base->total >= inet_peer_threshold)
>> 526 /* Remove one less-recently-used entry. */
>> 527 cleanup_once(0, stack);
>> 528
>> 529 return p;
>> 530 }
>> 531
>> 532 static int compute_total(void)
>> 533 {
>> 534 return v4_peers.total + v6_peers.total;
>>
>
> I really begin to think we have a bug here...
>
> In previous reports, I suggested to use slub_nomerge because I
> thought
> one corruption from another kernel layer was going on.
>
> (inetpeer was using 64 bytes objects). But now that inetpeer objects
> are
> bigger and sit in another kmemcache, its bad news.
>
> Could you try this, and eventually add some SLUB debugging stuff as
> well ?
Yes, i will try. I should enable SLUB debugging only, or also anything
else?
But possible it will take time to reproduce bug, seems it is occuring
rare. With 2.6.39 release i will rollout update to few hundreds PPPoE's,
maybe it will increase chances to get information.
^ permalink raw reply
* [Patch net-next-2.6] netpoll: disable netpoll when enslave a device
From: Amerigo Wang @ 2011-05-18 10:00 UTC (permalink / raw)
To: linux-kernel
Cc: WANG Cong, Neil Horman, Jay Vosburgh, Andy Gospodarek,
David S. Miller, Neil Horman, Alexey Dobriyan, Ferenc Wagner,
Andrew Morton, Paul E. McKenney, Josh Triplett, Ian Campbell,
netdev
Currently we do nothing when we enslave a net device which is running netconsole.
Neil pointed out that we may get weird results in such case, so let's disable
netpoll on the device being enslaved. I think it is too harsh to prevent
the device being ensalved if it is running netconsole.
By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
netdev notifier, because netpoll will check if the device is running or not
and we don't handle NETDEV_PRE_UP neither.
Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Neil Horman <nhorman@redhat.com>
---
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 088fd84..b9c70c5 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
}
}
+ netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
+
/* If this is the first slave, then we need to set the master's hardware
* address to be the same as the slave's. */
if (is_zero_ether_addr(bond->dev->dev_addr))
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index a83e101..0c3e8de 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -621,7 +621,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
bool stopped = false;
if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
- event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
+ event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN ||
+ event == NETDEV_ENSLAVE))
goto done;
spin_lock_irqsave(&target_list_lock, flags);
@@ -650,8 +651,8 @@ restart:
goto restart;
}
/* Fall through */
- case NETDEV_GOING_DOWN:
case NETDEV_BONDING_DESLAVE:
+ case NETDEV_ENSLAVE:
nt->enabled = 0;
stopped = true;
break;
@@ -660,10 +661,21 @@ restart:
netconsole_target_put(nt);
}
spin_unlock_irqrestore(&target_list_lock, flags);
- if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
+ if (stopped) {
printk(KERN_INFO "netconsole: network logging stopped on "
- "interface %s as it %s\n", dev->name,
- event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
+ "interface %s as it ", dev->name);
+ switch (event) {
+ case NETDEV_UNREGISTER:
+ printk(KERN_CONT "unregistered\n");
+ break;
+ case NETDEV_BONDING_DESLAVE:
+ printk(KERN_CONT "released slaves\n");
+ break;
+ case NETDEV_ENSLAVE:
+ printk(KERN_CONT "is enslaved\n");
+ break;
+ }
+ }
done:
return NOTIFY_DONE;
diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 621dfa1..3d82867 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
#define NETDEV_UNREGISTER_BATCH 0x0011
#define NETDEV_BONDING_DESLAVE 0x0012
#define NETDEV_NOTIFY_PEERS 0x0013
+#define NETDEV_ENSLAVE 0x0014
#define SYS_DOWN 0x0001 /* Notify of system down */
#define SYS_RESTART SYS_DOWN
^ permalink raw reply related
* Re: Bug, kernel panic, NULL dereference , cleanup_once / icmp_route_lookup.clone.19.clone / nat , 2.6.39-rc7-git11
From: Eric Dumazet @ 2011-05-18 10:05 UTC (permalink / raw)
To: Denys Fedoryshchenko; +Cc: netdev
In-Reply-To: <0a630bbf006d1210220a6ba4d55c5804@visp.net.lb>
Le mercredi 18 mai 2011 à 12:53 +0300, Denys Fedoryshchenko a écrit :
> Yes, i will try. I should enable SLUB debugging only, or also anything
> else?
>
> But possible it will take time to reproduce bug, seems it is occuring
> rare. With 2.6.39 release i will rollout update to few hundreds PPPoE's,
> maybe it will increase chances to get information.
>
I would try both : slub_debug=ZFP slub_nomerge
or maybe only slub_debug=ZFPU
Thanks !
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michael S. Tsirkin @ 2011-05-18 10:38 UTC (permalink / raw)
To: Shirley Ma
Cc: Michał Mirosław, Ben Hutchings, David Miller,
Eric Dumazet, Avi Kivity, Arnd Bergmann, netdev, kvm,
linux-kernel
In-Reply-To: <1305671318.10756.49.camel@localhost.localdomain>
On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> > > Hello Michael,
> > >
> > > Looks like to use a new flag requires more time/work. I am thinking
> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> > to
> > > avoid the new flag for now since mavctap uses real NICs as lower
> > device?
> >
> > Is there any other restriction besides requiring driver to not recycle
> > the skb? Are there any drivers that recycle TX skbs?
Not just recycling skbs, keeping reference to any of the pages in the
skb. Another requirement is to invoke the callback
in a timely fashion. For example virtio-net doesn't limit the time until
that happens (skbs are only freed when some other packet is
transmitted), so we need to avoid zcopy for such (nested-virt)
scenarious, right?
>
> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> OK to me from code review.
Hmm. pskb_expand_head calls skb_release_data while keeping
references to pages. How is that ok? What do I miss?
> Currently there is no drivers recycle TX skbs.
>
> Thanks
> Shirley
Well, with e.g. bridge or veth the skb might enter
the host networking stack. Once that happens, we lose
track of the pages. Or is there anything that
prevents such setups?
--
MST
^ permalink raw reply
* [PATCH] tcp: Expose the initial RTO via a new sysctl.
From: Benoit Sigoure @ 2011-05-18 10:43 UTC (permalink / raw)
To: davem, kuznet, pekkas, jmorris, yoshfuji, kaber
Cc: netdev, linux-kernel, Benoit Sigoure
In-Reply-To: <1305619677.2850.11.camel@edumazet-laptop>
Instead of hardcoding the initial RTO to 3s and requiring
the kernel to be recompiled to change it, expose it as a
sysctl that can be tuned at runtime. Leave the default
value unchanged.
Signed-off-by: Benoit Sigoure <tsunanet@gmail.com>
---
v2 of the patch to address Eric's comments. Of course I had to forget
to convert things back and forth between jiffies and ms -- /me n00b.
Code compiles. It seems like no one is opposed to this change, but if
one of you guys could express explicit interest in merging this change,
I'd be happy to spend a bit more time to test it.
The new sysctl is exposed in milliseconds but internally the value remains
in jiffies to avoid having to convert back / and forth between jiffies and
ms in most places.
I'm glad to hear that the default value will be tuned down to 1s. This
change will help people play with this value and easily revert it back at
runtime if they feel like they preferred the current value.
Thank you for your time.
Documentation/networking/ip-sysctl.txt | 8 ++++++++
include/net/tcp.h | 3 ++-
net/ipv4/syncookies.c | 2 +-
net/ipv4/sysctl_net_ipv4.c | 11 +++++++++++
net/ipv4/tcp.c | 4 ++--
net/ipv4/tcp_input.c | 8 ++++----
net/ipv4/tcp_ipv4.c | 6 +++---
net/ipv4/tcp_minisocks.c | 6 +++---
net/ipv4/tcp_output.c | 2 +-
net/ipv4/tcp_timer.c | 9 +++++----
net/ipv6/syncookies.c | 2 +-
net/ipv6/tcp_ipv6.c | 6 +++---
12 files changed, 44 insertions(+), 23 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index d3d653a..7f3c7d2 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -384,6 +384,14 @@ tcp_retries2 - INTEGER
RFC 1122 recommends at least 100 seconds for the timeout,
which corresponds to a value of at least 8.
+tcp_initial_rto - INTEGER
+ This value sets the initial retransmit timeout (in milliseconds),
+ that is how long the kernel will wait before retransmitting the
+ initial SYN packet.
+
+ RFC 1122 says that this SHOULD be 3000 milliseconds, which is the
+ default.
+
tcp_rfc1337 - BOOLEAN
If set, the TCP stack behaves conforming to RFC1337. If unset,
we are not conforming to RFC, but prevent TCP TIME_WAIT
diff --git a/include/net/tcp.h b/include/net/tcp.h
index cda30ea..d6d7dea 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -213,6 +213,7 @@ extern int sysctl_tcp_syn_retries;
extern int sysctl_tcp_synack_retries;
extern int sysctl_tcp_retries1;
extern int sysctl_tcp_retries2;
+extern int sysctl_tcp_initial_rto; /* in jiffies */
extern int sysctl_tcp_orphan_retries;
extern int sysctl_tcp_syncookies;
extern int sysctl_tcp_retrans_collapse;
@@ -295,7 +296,7 @@ static inline void tcp_synq_overflow(struct sock *sk)
static inline int tcp_synq_no_recent_overflow(const struct sock *sk)
{
unsigned long last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
- return time_after(jiffies, last_overflow + TCP_TIMEOUT_INIT);
+ return time_after(jiffies, last_overflow + sysctl_tcp_initial_rto);
}
extern struct proto tcp_prot;
diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
index 8b44c6d..b035968 100644
--- a/net/ipv4/syncookies.c
+++ b/net/ipv4/syncookies.c
@@ -186,7 +186,7 @@ __u32 cookie_v4_init_sequence(struct sock *sk, struct sk_buff *skb, __u16 *mssp)
* sysctl_tcp_retries1. It's a rather complicated formula (exponential
* backoff) to compute at runtime so it's currently hardcoded here.
*/
-#define COUNTER_TRIES 4
+#define COUNTER_TRIES (sysctl_tcp_initial_rto/HZ + 1)
/*
* Check if a ack sequence number is a valid syncookie.
* Return the decoded mss if it is, or 0 if not.
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 321e6e8..51c778d 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -30,6 +30,8 @@ static int tcp_adv_win_scale_min = -31;
static int tcp_adv_win_scale_max = 31;
static int ip_ttl_min = 1;
static int ip_ttl_max = 255;
+static int tcp_initial_rto_min = TCP_RTO_MIN;
+static int tcp_initial_rto_max = TCP_RTO_MAX;
/* Update system visible IP port range */
static void set_local_port_range(int range[2])
@@ -246,6 +248,15 @@ static struct ctl_table ipv4_table[] = {
.mode = 0644,
.proc_handler = proc_dointvec
},
+ {
+ .procname = "tcp_initial_rto",
+ .data = &sysctl_tcp_initial_rto,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_ms_jiffies,
+ .extra1 = &tcp_initial_rto_min,
+ .extra2 = &tcp_initial_rto_max,
+ },
{
.procname = "tcp_fin_timeout",
.data = &sysctl_tcp_fin_timeout,
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index b22d450..e9e7c3f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2352,7 +2352,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
case TCP_DEFER_ACCEPT:
/* Translate value in seconds to number of retransmits */
icsk->icsk_accept_queue.rskq_defer_accept =
- secs_to_retrans(val, TCP_TIMEOUT_INIT / HZ,
+ secs_to_retrans(val, sysctl_tcp_initial_rto / HZ,
TCP_RTO_MAX / HZ);
break;
@@ -2539,7 +2539,7 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
break;
case TCP_DEFER_ACCEPT:
val = retrans_to_secs(icsk->icsk_accept_queue.rskq_defer_accept,
- TCP_TIMEOUT_INIT / HZ, TCP_RTO_MAX / HZ);
+ sysctl_tcp_initial_rto / HZ, TCP_RTO_MAX / HZ);
break;
case TCP_WINDOW_CLAMP:
val = tp->window_clamp;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bef9f04..39f6c27 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -890,7 +890,7 @@ static void tcp_init_metrics(struct sock *sk)
if (dst_metric(dst, RTAX_RTT) == 0)
goto reset;
- if (!tp->srtt && dst_metric_rtt(dst, RTAX_RTT) < (TCP_TIMEOUT_INIT << 3))
+ if (!tp->srtt && dst_metric_rtt(dst, RTAX_RTT) < (sysctl_tcp_initial_rto << 3))
goto reset;
/* Initial rtt is determined from SYN,SYN-ACK.
@@ -916,7 +916,7 @@ static void tcp_init_metrics(struct sock *sk)
tp->mdev_max = tp->rttvar = max(tp->mdev, tcp_rto_min(sk));
}
tcp_set_rto(sk);
- if (inet_csk(sk)->icsk_rto < TCP_TIMEOUT_INIT && !tp->rx_opt.saw_tstamp) {
+ if (inet_csk(sk)->icsk_rto < sysctl_tcp_initial_rto && !tp->rx_opt.saw_tstamp) {
reset:
/* Play conservative. If timestamps are not
* supported, TCP will fail to recalculate correct
@@ -924,8 +924,8 @@ reset:
*/
if (!tp->rx_opt.saw_tstamp && tp->srtt) {
tp->srtt = 0;
- tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
- inet_csk(sk)->icsk_rto = TCP_TIMEOUT_INIT;
+ tp->mdev = tp->mdev_max = tp->rttvar = sysctl_tcp_initial_rto;
+ inet_csk(sk)->icsk_rto = sysctl_tcp_initial_rto;
}
}
tp->snd_cwnd = tcp_init_cwnd(tp, dst);
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index f7e6c2c..21920e6 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1383,7 +1383,7 @@ int tcp_v4_conn_request(struct sock *sk, struct sk_buff *skb)
want_cookie)
goto drop_and_free;
- inet_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
+ inet_csk_reqsk_queue_hash_add(sk, req, sysctl_tcp_initial_rto);
return 0;
drop_and_release:
@@ -1834,8 +1834,8 @@ static int tcp_v4_init_sock(struct sock *sk)
tcp_init_xmit_timers(sk);
tcp_prequeue_init(tp);
- icsk->icsk_rto = TCP_TIMEOUT_INIT;
- tp->mdev = TCP_TIMEOUT_INIT;
+ icsk->icsk_rto = sysctl_tcp_initial_rto;
+ tp->mdev = sysctl_tcp_initial_rto;
/* So many TCP implementations out there (incorrectly) count the
* initial SYN frame in their delayed-ACK and congestion control
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 80b1f80..c63ffa0 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -472,8 +472,8 @@ struct sock *tcp_create_openreq_child(struct sock *sk, struct request_sock *req,
tcp_init_wl(newtp, treq->rcv_isn);
newtp->srtt = 0;
- newtp->mdev = TCP_TIMEOUT_INIT;
- newicsk->icsk_rto = TCP_TIMEOUT_INIT;
+ newtp->mdev = sysctl_tcp_initial_rto;
+ newicsk->icsk_rto = sysctl_tcp_initial_rto;
newtp->packets_out = 0;
newtp->retrans_out = 0;
@@ -582,7 +582,7 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
* it can be estimated (approximately)
* from another data.
*/
- tmp_opt.ts_recent_stamp = get_seconds() - ((TCP_TIMEOUT_INIT/HZ)<<req->retrans);
+ tmp_opt.ts_recent_stamp = get_seconds() - ((sysctl_tcp_initial_rto/HZ)<<req->retrans);
paws_reject = tcp_paws_reject(&tmp_opt, th->rst);
}
}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 17388c7..e34b0f6 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2599,7 +2599,7 @@ static void tcp_connect_init(struct sock *sk)
tp->rcv_wup = 0;
tp->copied_seq = 0;
- inet_csk(sk)->icsk_rto = TCP_TIMEOUT_INIT;
+ inet_csk(sk)->icsk_rto = sysctl_tcp_initial_rto;
inet_csk(sk)->icsk_retransmits = 0;
tcp_clear_retrans(tp);
}
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index ecd44b0..b9da62b 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -29,6 +29,7 @@ int sysctl_tcp_keepalive_probes __read_mostly = TCP_KEEPALIVE_PROBES;
int sysctl_tcp_keepalive_intvl __read_mostly = TCP_KEEPALIVE_INTVL;
int sysctl_tcp_retries1 __read_mostly = TCP_RETR1;
int sysctl_tcp_retries2 __read_mostly = TCP_RETR2;
+int sysctl_tcp_initial_rto __read_mostly = TCP_TIMEOUT_INIT;
int sysctl_tcp_orphan_retries __read_mostly;
int sysctl_tcp_thin_linear_timeouts __read_mostly;
@@ -135,8 +136,8 @@ static void tcp_mtu_probing(struct inet_connection_sock *icsk, struct sock *sk)
/* This function calculates a "timeout" which is equivalent to the timeout of a
* TCP connection after "boundary" unsuccessful, exponentially backed-off
- * retransmissions with an initial RTO of TCP_RTO_MIN or TCP_TIMEOUT_INIT if
- * syn_set flag is set.
+ * retransmissions with an initial RTO of TCP_RTO_MIN or
+ * sysctl_tcp_initial_rto if syn_set flag is set.
*/
static bool retransmits_timed_out(struct sock *sk,
unsigned int boundary,
@@ -144,7 +145,7 @@ static bool retransmits_timed_out(struct sock *sk,
bool syn_set)
{
unsigned int linear_backoff_thresh, start_ts;
- unsigned int rto_base = syn_set ? TCP_TIMEOUT_INIT : TCP_RTO_MIN;
+ unsigned int rto_base = syn_set ? sysctl_tcp_initial_rto : TCP_RTO_MIN;
if (!inet_csk(sk)->icsk_retransmits)
return false;
@@ -495,7 +496,7 @@ out_unlock:
static void tcp_synack_timer(struct sock *sk)
{
inet_csk_reqsk_queue_prune(sk, TCP_SYNQ_INTERVAL,
- TCP_TIMEOUT_INIT, TCP_RTO_MAX);
+ sysctl_tcp_initial_rto, TCP_RTO_MAX);
}
void tcp_syn_ack_timeout(struct sock *sk, struct request_sock *req)
diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
index 352c260..f8a07a8 100644
--- a/net/ipv6/syncookies.c
+++ b/net/ipv6/syncookies.c
@@ -45,7 +45,7 @@ static __u16 const msstab[] = {
* sysctl_tcp_retries1. It's a rather complicated formula (exponential
* backoff) to compute at runtime so it's currently hardcoded here.
*/
-#define COUNTER_TRIES 4
+#define COUNTER_TRIES (sysctl_tcp_initial_rto/HZ + 1)
static inline struct sock *get_cookie_sock(struct sock *sk, struct sk_buff *skb,
struct request_sock *req,
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 4f49e5d..7e791e6 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1349,7 +1349,7 @@ have_isn:
want_cookie)
goto drop_and_free;
- inet6_csk_reqsk_queue_hash_add(sk, req, TCP_TIMEOUT_INIT);
+ inet6_csk_reqsk_queue_hash_add(sk, req, sysctl_tcp_initial_rto);
return 0;
drop_and_release:
@@ -1957,8 +1957,8 @@ static int tcp_v6_init_sock(struct sock *sk)
tcp_init_xmit_timers(sk);
tcp_prequeue_init(tp);
- icsk->icsk_rto = TCP_TIMEOUT_INIT;
- tp->mdev = TCP_TIMEOUT_INIT;
+ icsk->icsk_rto = sysctl_tcp_initial_rto;
+ tp->mdev = sysctl_tcp_initial_rto;
/* So many TCP implementations out there (incorrectly) count the
* initial SYN frame in their delayed-ACK and congestion control
--
1.7.0.4
^ permalink raw reply related
* Re: [Patch net-next-2.6] netpoll: disable netpoll when enslave a device
From: Neil Horman @ 2011-05-18 10:56 UTC (permalink / raw)
To: Amerigo Wang
Cc: linux-kernel, Neil Horman, Jay Vosburgh, Andy Gospodarek,
David S. Miller, Alexey Dobriyan, Ferenc Wagner, Andrew Morton,
Paul E. McKenney, Josh Triplett, Ian Campbell, netdev
In-Reply-To: <1305712845-11762-1-git-send-email-amwang@redhat.com>
On Wed, May 18, 2011 at 06:00:35PM +0800, Amerigo Wang wrote:
> Currently we do nothing when we enslave a net device which is running netconsole.
> Neil pointed out that we may get weird results in such case, so let's disable
> netpoll on the device being enslaved. I think it is too harsh to prevent
> the device being ensalved if it is running netconsole.
>
> By the way, this patch also removes the NETDEV_GOING_DOWN from netconsole
> netdev notifier, because netpoll will check if the device is running or not
> and we don't handle NETDEV_PRE_UP neither.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Neil Horman <nhorman@redhat.com>
>
> ---
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 088fd84..b9c70c5 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1640,6 +1640,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> }
> }
>
> + netdev_bonding_change(slave_dev, NETDEV_ENSLAVE);
> +
> /* If this is the first slave, then we need to set the master's hardware
> * address to be the same as the slave's. */
> if (is_zero_ether_addr(bond->dev->dev_addr))
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index a83e101..0c3e8de 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -621,7 +621,8 @@ static int netconsole_netdev_event(struct notifier_block *this,
> bool stopped = false;
>
> if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER ||
> - event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN))
> + event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN ||
> + event == NETDEV_ENSLAVE))
> goto done;
>
> spin_lock_irqsave(&target_list_lock, flags);
> @@ -650,8 +651,8 @@ restart:
> goto restart;
> }
> /* Fall through */
> - case NETDEV_GOING_DOWN:
> case NETDEV_BONDING_DESLAVE:
> + case NETDEV_ENSLAVE:
> nt->enabled = 0;
> stopped = true;
> break;
This wasn't introduced by this patch, but looking at it made me realize that
nt->enabled, if it passes through this code path, doesn't properly track weather
or not netpoll_setup has been called on this interface. If you look at
drop_netconsole_target, you'll see we only call netpoll_cleanup_target if
nt->enabled is set. We should probably change the nt->enabled check there, and
in store_enabled to be if (nt->np.dev), like we do in the NETDEV_UNREGISTER case
in netconsole_netdev_event.
> @@ -660,10 +661,21 @@ restart:
> netconsole_target_put(nt);
> }
> spin_unlock_irqrestore(&target_list_lock, flags);
> - if (stopped && (event == NETDEV_UNREGISTER || event == NETDEV_BONDING_DESLAVE))
> + if (stopped) {
> printk(KERN_INFO "netconsole: network logging stopped on "
> - "interface %s as it %s\n", dev->name,
> - event == NETDEV_UNREGISTER ? "unregistered" : "released slaves");
> + "interface %s as it ", dev->name);
> + switch (event) {
> + case NETDEV_UNREGISTER:
> + printk(KERN_CONT "unregistered\n");
> + break;
> + case NETDEV_BONDING_DESLAVE:
> + printk(KERN_CONT "released slaves\n");
> + break;
> + case NETDEV_ENSLAVE:
> + printk(KERN_CONT "is enslaved\n");
> + break;
> + }
> + }
>
> done:
> return NOTIFY_DONE;
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 621dfa1..3d82867 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -211,6 +211,7 @@ static inline int notifier_to_errno(int ret)
> #define NETDEV_UNREGISTER_BATCH 0x0011
> #define NETDEV_BONDING_DESLAVE 0x0012
> #define NETDEV_NOTIFY_PEERS 0x0013
> +#define NETDEV_ENSLAVE 0x0014
>
Nit:
Shouldn't this be NETDEV_BONDING_ENSLAVE, to keep it in line with
NETDEV_BONDING_DESLAVE above?
> #define SYS_DOWN 0x0001 /* Notify of system down */
> #define SYS_RESTART SYS_DOWN
>
Other than those two points, this looks good to me
Thanks!
Neil
^ permalink raw reply
* [PATCH net-next] bnx2x: add support for retrieving dcb peer configuration
From: Shmulik Ravid @ 2011-05-18 12:55 UTC (permalink / raw)
To: davem; +Cc: Eilon Greenstein, netdev
This patch adds support to the bnx2x for retrieving dcb peer (remote)
configuration from the embedded DCBX stack.
Signed-off-by: Shmulik Ravid <shmulikr@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/bnx2x/bnx2x.h | 4 +
drivers/net/bnx2x/bnx2x_dcb.c | 152 ++++++++++++++++++++++++++++++++++-------
2 files changed, 131 insertions(+), 25 deletions(-)
diff --git a/drivers/net/bnx2x/bnx2x.h b/drivers/net/bnx2x/bnx2x.h
index 16a76f0..668a578 100644
--- a/drivers/net/bnx2x/bnx2x.h
+++ b/drivers/net/bnx2x/bnx2x.h
@@ -1242,6 +1242,10 @@ struct bnx2x {
/* DCBX Negotiation results */
struct dcbx_features dcbx_local_feat;
u32 dcbx_error;
+#ifdef BCM_DCBNL
+ struct dcbx_features dcbx_remote_feat;
+ u32 dcbx_remote_flags;
+#endif
u32 pending_max;
};
diff --git a/drivers/net/bnx2x/bnx2x_dcb.c b/drivers/net/bnx2x/bnx2x_dcb.c
index 0f83092..410a49e 100644
--- a/drivers/net/bnx2x/bnx2x_dcb.c
+++ b/drivers/net/bnx2x/bnx2x_dcb.c
@@ -485,6 +485,36 @@ static void bnx2x_dcbx_update_ets_params(struct bnx2x *bp)
}
}
+#ifdef BCM_DCBNL
+static int bnx2x_dcbx_read_shmem_remote_mib(struct bnx2x *bp)
+{
+ struct lldp_remote_mib remote_mib = {0};
+ u32 dcbx_remote_mib_offset = SHMEM2_RD(bp, dcbx_remote_mib_offset);
+ int rc;
+
+ DP(NETIF_MSG_LINK, "dcbx_remote_mib_offset 0x%x\n",
+ dcbx_remote_mib_offset);
+
+ if (SHMEM_DCBX_REMOTE_MIB_NONE == dcbx_remote_mib_offset) {
+ BNX2X_ERR("FW doesn't support dcbx_remote_mib_offset\n");
+ return -EINVAL;
+ }
+
+ rc = bnx2x_dcbx_read_mib(bp, (u32 *)&remote_mib, dcbx_remote_mib_offset,
+ DCBX_READ_REMOTE_MIB);
+
+ if (rc) {
+ BNX2X_ERR("Faild to read remote mib from FW\n");
+ return rc;
+ }
+
+ /* save features and flags */
+ bp->dcbx_remote_feat = remote_mib.features;
+ bp->dcbx_remote_flags = remote_mib.flags;
+ return 0;
+}
+#endif
+
static int bnx2x_dcbx_read_shmem_neg_results(struct bnx2x *bp)
{
struct lldp_local_mib local_mib = {0};
@@ -601,6 +631,10 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32 state)
* negotiation results
*/
bnx2x_dcbnl_update_applist(bp, true);
+
+ /* Read rmeote mib if dcbx is in the FW */
+ if (bnx2x_dcbx_read_shmem_remote_mib(bp))
+ return;
#endif
/* Read neg results if dcbx is in the FW */
if (bnx2x_dcbx_read_shmem_neg_results(bp))
@@ -2031,7 +2065,6 @@ static u8 bnx2x_dcbnl_set_dcbx(struct net_device *netdev, u8 state)
return 0;
}
-
static u8 bnx2x_dcbnl_get_featcfg(struct net_device *netdev, int featid,
u8 *flags)
{
@@ -2111,31 +2144,100 @@ static u8 bnx2x_dcbnl_set_featcfg(struct net_device *netdev, int featid,
return rval;
}
+static int bnx2x_peer_appinfo(struct net_device *netdev,
+ struct dcb_peer_app_info *info, u16* app_count)
+{
+ int i;
+ struct bnx2x *bp = netdev_priv(netdev);
+
+ DP(NETIF_MSG_LINK, "APP-INFO\n");
+
+ info->willing = (bp->dcbx_remote_flags & DCBX_APP_REM_WILLING) ?: 0;
+ info->error = (bp->dcbx_remote_flags & DCBX_APP_RX_ERROR) ?: 0;
+ *app_count = 0;
+
+ for (i = 0; i < DCBX_MAX_APP_PROTOCOL; i++)
+ if (bp->dcbx_remote_feat.app.app_pri_tbl[i].appBitfield &
+ DCBX_APP_ENTRY_VALID)
+ (*app_count)++;
+ return 0;
+}
+
+static int bnx2x_peer_apptable(struct net_device *netdev,
+ struct dcb_app *table)
+{
+ int i, j;
+ struct bnx2x *bp = netdev_priv(netdev);
+
+ DP(NETIF_MSG_LINK, "APP-TABLE\n");
+
+ for (i = 0, j = 0; i < DCBX_MAX_APP_PROTOCOL; i++) {
+ struct dcbx_app_priority_entry *ent =
+ &bp->dcbx_remote_feat.app.app_pri_tbl[i];
+
+ if (ent->appBitfield & DCBX_APP_ENTRY_VALID) {
+ table[j].selector = bnx2x_dcbx_dcbnl_app_idtype(ent);
+ table[j].priority = bnx2x_dcbx_dcbnl_app_up(ent);
+ table[j++].protocol = ent->app_id;
+ }
+ }
+ return 0;
+}
+
+static int bnx2x_cee_peer_getpg(struct net_device *netdev, struct cee_pg *pg)
+{
+ int i;
+ struct bnx2x *bp = netdev_priv(netdev);
+
+ pg->willing = (bp->dcbx_remote_flags & DCBX_ETS_REM_WILLING) ?: 0;
+
+ for (i = 0; i < CEE_DCBX_MAX_PGS; i++) {
+ pg->pg_bw[i] =
+ DCBX_PG_BW_GET(bp->dcbx_remote_feat.ets.pg_bw_tbl, i);
+ pg->prio_pg[i] =
+ DCBX_PRI_PG_GET(bp->dcbx_remote_feat.ets.pri_pg_tbl, i);
+ }
+ return 0;
+}
+
+static int bnx2x_cee_peer_getpfc(struct net_device *netdev,
+ struct cee_pfc *pfc)
+{
+ struct bnx2x *bp = netdev_priv(netdev);
+ pfc->tcs_supported = bp->dcbx_remote_feat.pfc.pfc_caps;
+ pfc->pfc_en = bp->dcbx_remote_feat.pfc.pri_en_bitmap;
+ return 0;
+}
+
const struct dcbnl_rtnl_ops bnx2x_dcbnl_ops = {
- .getstate = bnx2x_dcbnl_get_state,
- .setstate = bnx2x_dcbnl_set_state,
- .getpermhwaddr = bnx2x_dcbnl_get_perm_hw_addr,
- .setpgtccfgtx = bnx2x_dcbnl_set_pg_tccfg_tx,
- .setpgbwgcfgtx = bnx2x_dcbnl_set_pg_bwgcfg_tx,
- .setpgtccfgrx = bnx2x_dcbnl_set_pg_tccfg_rx,
- .setpgbwgcfgrx = bnx2x_dcbnl_set_pg_bwgcfg_rx,
- .getpgtccfgtx = bnx2x_dcbnl_get_pg_tccfg_tx,
- .getpgbwgcfgtx = bnx2x_dcbnl_get_pg_bwgcfg_tx,
- .getpgtccfgrx = bnx2x_dcbnl_get_pg_tccfg_rx,
- .getpgbwgcfgrx = bnx2x_dcbnl_get_pg_bwgcfg_rx,
- .setpfccfg = bnx2x_dcbnl_set_pfc_cfg,
- .getpfccfg = bnx2x_dcbnl_get_pfc_cfg,
- .setall = bnx2x_dcbnl_set_all,
- .getcap = bnx2x_dcbnl_get_cap,
- .getnumtcs = bnx2x_dcbnl_get_numtcs,
- .setnumtcs = bnx2x_dcbnl_set_numtcs,
- .getpfcstate = bnx2x_dcbnl_get_pfc_state,
- .setpfcstate = bnx2x_dcbnl_set_pfc_state,
- .setapp = bnx2x_dcbnl_set_app_up,
- .getdcbx = bnx2x_dcbnl_get_dcbx,
- .setdcbx = bnx2x_dcbnl_set_dcbx,
- .getfeatcfg = bnx2x_dcbnl_get_featcfg,
- .setfeatcfg = bnx2x_dcbnl_set_featcfg,
+ .getstate = bnx2x_dcbnl_get_state,
+ .setstate = bnx2x_dcbnl_set_state,
+ .getpermhwaddr = bnx2x_dcbnl_get_perm_hw_addr,
+ .setpgtccfgtx = bnx2x_dcbnl_set_pg_tccfg_tx,
+ .setpgbwgcfgtx = bnx2x_dcbnl_set_pg_bwgcfg_tx,
+ .setpgtccfgrx = bnx2x_dcbnl_set_pg_tccfg_rx,
+ .setpgbwgcfgrx = bnx2x_dcbnl_set_pg_bwgcfg_rx,
+ .getpgtccfgtx = bnx2x_dcbnl_get_pg_tccfg_tx,
+ .getpgbwgcfgtx = bnx2x_dcbnl_get_pg_bwgcfg_tx,
+ .getpgtccfgrx = bnx2x_dcbnl_get_pg_tccfg_rx,
+ .getpgbwgcfgrx = bnx2x_dcbnl_get_pg_bwgcfg_rx,
+ .setpfccfg = bnx2x_dcbnl_set_pfc_cfg,
+ .getpfccfg = bnx2x_dcbnl_get_pfc_cfg,
+ .setall = bnx2x_dcbnl_set_all,
+ .getcap = bnx2x_dcbnl_get_cap,
+ .getnumtcs = bnx2x_dcbnl_get_numtcs,
+ .setnumtcs = bnx2x_dcbnl_set_numtcs,
+ .getpfcstate = bnx2x_dcbnl_get_pfc_state,
+ .setpfcstate = bnx2x_dcbnl_set_pfc_state,
+ .setapp = bnx2x_dcbnl_set_app_up,
+ .getdcbx = bnx2x_dcbnl_get_dcbx,
+ .setdcbx = bnx2x_dcbnl_set_dcbx,
+ .getfeatcfg = bnx2x_dcbnl_get_featcfg,
+ .setfeatcfg = bnx2x_dcbnl_set_featcfg,
+ .peer_getappinfo = bnx2x_peer_appinfo,
+ .peer_getapptable = bnx2x_peer_apptable,
+ .cee_peer_getpg = bnx2x_cee_peer_getpg,
+ .cee_peer_getpfc = bnx2x_cee_peer_getpfc,
};
#endif /* BCM_DCBNL */
--
1.7.3.5
^ permalink raw reply related
* Re: [PATCH] SCTP: fix race between sctp_bind_addr_free() and sctp_bind_addr_conflict()
From: Jacek Luczak @ 2011-05-18 11:01 UTC (permalink / raw)
To: Wei Yongjun; +Cc: Eric Dumazet, netdev, Vlad Yasevich
In-Reply-To: <4DD38B30.9090601@cn.fujitsu.com>
2011/5/18 Wei Yongjun <yjwei@cn.fujitsu.com>:
>
>> Le mercredi 18 mai 2011 à 10:06 +0200, Jacek Luczak a écrit :
>>> 2011/5/18 Eric Dumazet <eric.dumazet@gmail.com>:
>>>> If you're removing items from this list, you must be a writer here, with
>>>> exclusive access. So rcu_read_lock()/rcu_read_unlock() is not necessary.
>>> I could agree to some extend ... but strict RCU section IMO is needed here.
>>> I can check this if the issue exists.
>>>
>> I can tell you for sure rcu_read_lock() is not needed here. Its only
>> showing confusion from code's author.
>>
>> Please read Documentation/RCU/listRCU.txt for concise explanations,
>> line 117.
>>
>>
>>>> Therefore, I guess following code is better :
>>>>
>>>> list_for_each_entry(addr, &bp->address_list, list) {
>>>> list_del_rcu(&addr->list);
>>>> call_rcu(&addr->rcu, sctp_local_addr_free);
>>>> SCTP_DBG_OBJCNT_DEC(addr);
>>>> }
>>>>
>>>> Then, why dont you fix sctp_bind_addr_clean() instead ?
>>>>
>>>> if 'struct sctp_sockaddr_entry' is recu protected, then all frees should
>>>> be protected as well.
>>> The _clean() as claimed by Vlad is called many times from various places
>>> in code and this could give a overhead. I guess Vlad would need to comment.
>> I guess a full review of this code is needed. You'll have to prove
>> sctp_bind_addr_clean() is always called after one RCU grace period if
>> you want to leave it as is.
>>
>> You cant get RCU for free, some rules must be followed or you risk
>> crashes.
>>
>
> fix the race between sctp_bind_addr_free() and sctp_bind_addr_conflict(), maybe you just
> need to remove the socket from the port hash before empty the bind address list.
> some thing like this, not test.
>
> diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
> index c8cc24e..924d846 100644
> --- a/net/sctp/endpointola.c
> +++ b/net/sctp/endpointola.c
> @@ -268,12 +268,13 @@ static void sctp_endpoint_destroy(struct sctp_endpoint *ep)
>
> /* Cleanup. */
> sctp_inq_free(&ep->base.inqueue);
> - sctp_bind_addr_free(&ep->base.bind_addr);
>
> /* Remove and free the port */
> if (sctp_sk(ep->base.sk)->bind_hash)
> sctp_put_port(ep->base.sk);
>
> + sctp_bind_addr_free(&ep->base.bind_addr);
> +
> /* Give up our hold on the sock. */
> if (ep->base.sk)
> sock_put(ep->base.sk);
>
Done tests with that one and looks like it survive the flood.
How then this will be handled is up to you. Both ways fix
the issue which makes me happy as damn hell.
@Eric, if you will take a look into the code you might notice
that there are few places where list operations could be
optimised and the main question here is do we really care
to have the data ,,safe'' so that things like that won't popup.
The good example can be the set of _local_ functions.
Ahhh... and I'm aware of how tricky can be abuse of RCU.
-Jacek
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michał Mirosław @ 2011-05-18 11:10 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <20110518103819.GL7589@redhat.com>
2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
>> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
>> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
>> > > Hello Michael,
>> > >
>> > > Looks like to use a new flag requires more time/work. I am thinking
>> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
>> > to
>> > > avoid the new flag for now since mavctap uses real NICs as lower
>> > device?
>> >
>> > Is there any other restriction besides requiring driver to not recycle
>> > the skb? Are there any drivers that recycle TX skbs?
>
> Not just recycling skbs, keeping reference to any of the pages in the
> skb. Another requirement is to invoke the callback
> in a timely fashion. For example virtio-net doesn't limit the time until
> that happens (skbs are only freed when some other packet is
> transmitted), so we need to avoid zcopy for such (nested-virt)
> scenarious, right?
Hmm. But every hardware driver supporting SG will keep reference to
the pages until the packet is sent (or DMA'd to the device). This can
take a long time if hardware queue happens to stall for some reason.
Is it that you mean keeping a reference after all skbs pointing to the
pages are released?
>> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
>> OK to me from code review.
> Hmm. pskb_expand_head calls skb_release_data while keeping
> references to pages. How is that ok? What do I miss?
It's making copy of the skb_shinfo earlier, so the pages refcount
stays the same.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH V5 2/6 net-next] netdevice.h: Add zero-copy flag in netdevice
From: Michael S. Tsirkin @ 2011-05-18 11:17 UTC (permalink / raw)
To: Michał Mirosław
Cc: Shirley Ma, Ben Hutchings, David Miller, Eric Dumazet, Avi Kivity,
Arnd Bergmann, netdev, kvm, linux-kernel
In-Reply-To: <BANLkTimc1RbC3uQsra1HUvS_Trg63iMWGA@mail.gmail.com>
On Wed, May 18, 2011 at 01:10:50PM +0200, Michał Mirosław wrote:
> 2011/5/18 Michael S. Tsirkin <mst@redhat.com>:
> > On Tue, May 17, 2011 at 03:28:38PM -0700, Shirley Ma wrote:
> >> On Tue, 2011-05-17 at 23:48 +0200, Michał Mirosław wrote:
> >> > 2011/5/17 Shirley Ma <mashirle@us.ibm.com>:
> >> > > Hello Michael,
> >> > >
> >> > > Looks like to use a new flag requires more time/work. I am thinking
> >> > > whether we can just use HIGHDMA flag to enable zero-copy in macvtap
> >> > to
> >> > > avoid the new flag for now since mavctap uses real NICs as lower
> >> > device?
> >> >
> >> > Is there any other restriction besides requiring driver to not recycle
> >> > the skb? Are there any drivers that recycle TX skbs?
> >
> > Not just recycling skbs, keeping reference to any of the pages in the
> > skb. Another requirement is to invoke the callback
> > in a timely fashion. For example virtio-net doesn't limit the time until
> > that happens (skbs are only freed when some other packet is
> > transmitted), so we need to avoid zcopy for such (nested-virt)
> > scenarious, right?
>
> Hmm. But every hardware driver supporting SG will keep reference to
> the pages until the packet is sent (or DMA'd to the device). This can
> take a long time if hardware queue happens to stall for some reason.
That's a fundamental property of zero copy transmit.
You can't let the application/guest reuse the memory until
no one looks at it anymore.
> Is it that you mean keeping a reference after all skbs pointing to the
> pages are released?
No one should reference the pages after the callback is invoked, yes.
> >> Not more other restrictions, skb clone is OK. pskb_expand_head() looks
> >> OK to me from code review.
> > Hmm. pskb_expand_head calls skb_release_data while keeping
> > references to pages. How is that ok? What do I miss?
>
> It's making copy of the skb_shinfo earlier, so the pages refcount
> stays the same.
>
> Best Regards,
> Michał Mirosław
Exactly. But the callback is invoked so the guest thinks it's ok to
change this memory. If it does a corrupted packet will be sent out.
--
MST
^ 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