* Re: pull request: wireless-next-2.6 2009-10-28
From: Pavel Machek @ 2009-11-02 21:16 UTC (permalink / raw)
To: Luis Correia
Cc: John W. Linville, Ingo Molnar, Johannes Berg, Jarek Poplawski,
Bartlomiej Zolnierkiewicz, Pekka Enberg, David Miller,
linux-wireless, netdev, linux-kernel
In-Reply-To: <efe7343f0911020932l21a570c8u9478568329d38e2c@mail.gmail.com>
> About Bartlomiej, there is only one more thing that I'll say:
>
> I've searched on my GMail archives and the only patch Bart has
> provided so far for the rt2x00 project is this:
>
> [PATCH] MAINTAINERS: rt2x00 list is moderated
>
> Which, while technically correct, adds nothing to the project.
>
> So, I will personally continue to ignore Bart's comments, regards and
> rants, until he provides patches for rt2x00 that actually make the
> driver better.
Well, he provided review feedback... he should be thanked for that,
not flamed for it.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Martin Devera @ 2009-11-04 12:15 UTC (permalink / raw)
To: Changli Gao; +Cc: Jarek Poplawski, Jamal Hadi Salim, netdev
In-Reply-To: <412e6f7f0911040408w5adcdde8yc1add4a561e90e40@mail.gmail.com>
Changli Gao wrote:
> On Wed, Nov 4, 2009 at 7:58 PM, Martin Devera <martin.devera@cdi.cz> wrote:
>> Ahh I'm starting to understand. You are concerned mainly with
>>
>> cl->tokens += diff; /* we moved t_c; update tokens */
>>
>> am I right ?
>>
>
> If we don't need to charge the tokens, updating it is necessary. I am
> arguing the tokens isn't updated with the ctokens when the
> corresponding class is in HTB_MAY_BORROW mode.
>
Ok, then the reason is in "logic". When you have parent P and children
A,B each with half of rate and only A is sending, than we want A to use
its own rate and also B's rate. B's rate is available as excess in P.
Here I see correct to charge part to A and part to P, while you want
to charge one P and twice A. Only mbuffer will limit the duplicity.
And it is not good idea because tokens can go very negative and then
ittroduce A's stall for some time.
I see no principial benefit...
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Jarek Poplawski @ 2009-11-04 12:11 UTC (permalink / raw)
To: Changli Gao; +Cc: Jamal Hadi Salim, devik, netdev
In-Reply-To: <412e6f7f0911040401g5a8d58b0yc8c1637d419094f2@mail.gmail.com>
On Wed, Nov 04, 2009 at 08:01:23PM +0800, Changli Gao wrote:
> On Wed, Nov 4, 2009 at 7:49 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> > On Wed, Nov 04, 2009 at 07:21:48PM +0800, Changli Gao wrote:
> >>
> >> I don't think so. Although a class's tokens may be negative, but its
> >> ctokens may be positive. Charging its tokens is to prevent its cmode
> >> from being changed to HTB_CAN_SEND from HTB_CANT_SEND directly.
> >
> > I think, you should really better show some tests proving your patch
> > is needed and doesn't affect a case I described, instead of trying to
> > discuss the meaninig of all HTB variables here.
> >
>
> I test it before sending it here, but it doesn't show any obvious
> difference as Martin said, no worse and no better.
Strange... How this patch could be needed or even "necessary", and
do "no worse and no better" at the same time.
> I don't know how to
> construct a test to show you the bad effect you worry about. Any
> suggestion about the test?
E.g. something like I described, but only rates 50kbit and 100kbit
instead of 5 and 10 packets/sec.
Regards,
Jarek P.
^ permalink raw reply
* Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-04 12:10 UTC (permalink / raw)
To: Andi Kleen
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm
In-Reply-To: <878wema6o0.fsf@basil.nowhere.org>
On Wed, Nov 04, 2009 at 12:08:47PM +0100, Andi Kleen wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
> Haven't really read the whole thing, just noticed something at a glance.
>
> > +/* Expects to be always run from workqueue - which acts as
> > + * read-size critical section for our kind of RCU. */
> > +static void handle_tx(struct vhost_net *net)
> > +{
> > + struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> > + unsigned head, out, in, s;
> > + struct msghdr msg = {
> > + .msg_name = NULL,
> > + .msg_namelen = 0,
> > + .msg_control = NULL,
> > + .msg_controllen = 0,
> > + .msg_iov = vq->iov,
> > + .msg_flags = MSG_DONTWAIT,
> > + };
> > + size_t len, total_len = 0;
> > + int err, wmem;
> > + size_t hdr_size;
> > + struct socket *sock = rcu_dereference(vq->private_data);
> > + if (!sock)
> > + return;
> > +
> > + wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> > + if (wmem >= sock->sk->sk_sndbuf)
> > + return;
> > +
> > + use_mm(net->dev.mm);
>
> I haven't gone over all this code in detail, but that isolated reference count
> use looks suspicious. What prevents the mm from going away before
> you increment, if it's not the current one?
We take a reference to it before we start any virtqueues,
and stop all virtqueues before we drop the reference:
/* Caller should have device mutex */
static long vhost_dev_set_owner(struct vhost_dev *dev)
{
/* Is there an owner already? */
if (dev->mm)
return -EBUSY;
/* No owner, become one */
dev->mm = get_task_mm(current);
return 0;
}
And
vhost_dev_cleanup:
....
if (dev->mm)
mmput(dev->mm);
dev->mm = NULL;
}
Fine?
> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Changli Gao @ 2009-11-04 12:08 UTC (permalink / raw)
To: Martin Devera; +Cc: Jarek Poplawski, Jamal Hadi Salim, netdev
In-Reply-To: <4AF16C49.409@cdi.cz>
On Wed, Nov 4, 2009 at 7:58 PM, Martin Devera <martin.devera@cdi.cz> wrote:
>
> Ahh I'm starting to understand. You are concerned mainly with
>
> cl->tokens += diff; /* we moved t_c; update tokens */
>
> am I right ?
>
If we don't need to charge the tokens, updating it is necessary. I am
arguing the tokens isn't updated with the ctokens when the
corresponding class is in HTB_MAY_BORROW mode.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-04 12:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: Gregory Haskins, netdev, virtualization, kvm, linux-kernel, mingo,
linux-mm, akpm, hpa, Rusty Russell, s.hetze, Paul E. McKenney
In-Reply-To: <4AF09C70.6090505@gmail.com>
On Tue, Nov 03, 2009 at 10:11:12PM +0100, Eric Dumazet wrote:
> Michael S. Tsirkin a écrit :
> >
> > Paul, you acked this previously. Should I add you acked-by line so
> > people calm down? If you would rather I replace
> > rcu_dereference/rcu_assign_pointer with rmb/wmb, I can do this.
> > Or maybe patch Documentation to explain this RCU usage?
> >
>
> So you believe I am over-reacting to this dubious use of RCU ?
>
> RCU documentation is already very complex, we dont need to add yet another
> subtle use, and makes it less readable.
>
> It seems you use 'RCU api' in drivers/vhost/net.c as convenient macros :
>
> #define rcu_dereference(p) ({ \
> typeof(p) _________p1 = ACCESS_ONCE(p); \
> smp_read_barrier_depends(); \
> (_________p1); \
> })
>
> #define rcu_assign_pointer(p, v) \
> ({ \
> if (!__builtin_constant_p(v) || \
> ((v) != NULL)) \
> smp_wmb(); \
> (p) = (v); \
> })
>
>
> There are plenty regular uses of smp_wmb() in kernel, not related to Read Copy Update,
> there is nothing wrong to use barriers with appropriate comments.
Well, what I do has classic RCU characteristics: readers do not take
locks, writers take a lock and flush after update. This is why I believe
rcu_dereference and rcu_assign_pointer are more appropriate here than
open-coding barriers.
Before deciding whether it's a good idea to open-code barriers
instead, I would like to hear Paul's opinion.
>
> (And you already use mb(), wmb(), rmb(), smp_wmb() in your patch)
Yes, virtio guest pretty much forces this, there's no way to share
a lock with the guest.
> BTW there is at least one locking bug in vhost_net_set_features()
>
> Apparently, mutex_unlock() doesnt trigger a fault if mutex is not locked
> by current thread... even with DEBUG_MUTEXES / DEBUG_LOCK_ALLOC
>
>
> static void vhost_net_set_features(struct vhost_net *n, u64 features)
> {
> size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
> sizeof(struct virtio_net_hdr) : 0;
> int i;
> <<!>> mutex_unlock(&n->dev.mutex);
> n->dev.acked_features = features;
> smp_wmb();
> for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> mutex_lock(&n->vqs[i].mutex);
> n->vqs[i].hdr_size = hdr_size;
> mutex_unlock(&n->vqs[i].mutex);
> }
> mutex_unlock(&n->dev.mutex);
> vhost_net_flush(n);
> }
Thanks very much for spotting this! Will fix.
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Changli Gao @ 2009-11-04 12:01 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Jamal Hadi Salim, devik, netdev
In-Reply-To: <20091104114922.GA8176@ff.dom.local>
On Wed, Nov 4, 2009 at 7:49 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Wed, Nov 04, 2009 at 07:21:48PM +0800, Changli Gao wrote:
>>
>> I don't think so. Although a class's tokens may be negative, but its
>> ctokens may be positive. Charging its tokens is to prevent its cmode
>> from being changed to HTB_CAN_SEND from HTB_CANT_SEND directly.
>
> I think, you should really better show some tests proving your patch
> is needed and doesn't affect a case I described, instead of trying to
> discuss the meaninig of all HTB variables here.
>
I test it before sending it here, but it doesn't show any obvious
difference as Martin said, no worse and no better. I don't know how to
construct a test to show you the bad effect you worry about. Any
suggestion about the test?
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Martin Devera @ 2009-11-04 11:58 UTC (permalink / raw)
To: Changli Gao; +Cc: Jarek Poplawski, Jamal Hadi Salim, netdev
In-Reply-To: <412e6f7f0911040330uc5ebbcekfef57854fce501f@mail.gmail.com>
Changli Gao wrote:
> On Wed, Nov 4, 2009 at 6:58 PM, Martin Devera <martin.devera@cdi.cz> wrote:
>> Hello,
>>
>> yes you are right. If I remember correctly, I tried to charge "rate"
>> tokens up to root in early versions (which makes some sense) but problem
>> is with many interior classes stuck at extreme mbuffer value, losing
>> responsivity.
>
> It is strange. Would you like describe it more clearly?
Ahh I'm starting to understand. You are concerned mainly with
cl->tokens += diff; /* we moved t_c; update tokens */
am I right ?
^ permalink raw reply
* Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server
From: Michael S. Tsirkin @ 2009-11-04 11:57 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Gregory Haskins, Eric Dumazet, netdev, virtualization, kvm,
linux-kernel, mingo, linux-mm, akpm, hpa, Rusty Russell, s.hetze
In-Reply-To: <20091103235744.GF6726@linux.vnet.ibm.com>
On Tue, Nov 03, 2009 at 03:57:44PM -0800, Paul E. McKenney wrote:
> On Tue, Nov 03, 2009 at 01:14:06PM -0500, Gregory Haskins wrote:
> > Gregory Haskins wrote:
> > > Eric Dumazet wrote:
> > >> Michael S. Tsirkin a écrit :
> > >>> +static void handle_tx(struct vhost_net *net)
> > >>> +{
> > >>> + struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> > >>> + unsigned head, out, in, s;
> > >>> + struct msghdr msg = {
> > >>> + .msg_name = NULL,
> > >>> + .msg_namelen = 0,
> > >>> + .msg_control = NULL,
> > >>> + .msg_controllen = 0,
> > >>> + .msg_iov = vq->iov,
> > >>> + .msg_flags = MSG_DONTWAIT,
> > >>> + };
> > >>> + size_t len, total_len = 0;
> > >>> + int err, wmem;
> > >>> + size_t hdr_size;
> > >>> + struct socket *sock = rcu_dereference(vq->private_data);
> > >>> + if (!sock)
> > >>> + return;
> > >>> +
> > >>> + wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> > >>> + if (wmem >= sock->sk->sk_sndbuf)
> > >>> + return;
> > >>> +
> > >>> + use_mm(net->dev.mm);
> > >>> + mutex_lock(&vq->mutex);
> > >>> + vhost_no_notify(vq);
> > >>> +
> > >> using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect
> > >> that your use of RCU is not correct.
> > >>
> > >> 1) rcu_dereference() should be done inside a read_rcu_lock() section, and
> > >> we are not allowed to sleep in such a section.
> > >> (Quoting Documentation/RCU/whatisRCU.txt :
> > >> It is illegal to block while in an RCU read-side critical section, )
> > >>
> > >> 2) mutex_lock() can sleep (ie block)
> > >>
> > >
> > >
> > > Michael,
> > > I warned you that this needed better documentation ;)
> > >
> > > Eric,
> > > I think I flagged this once before, but Michael convinced me that it
> > > was indeed "ok", if but perhaps a bit unconventional. I will try to
> > > find the thread.
> > >
> > > Kind Regards,
> > > -Greg
> > >
> >
> > Here it is:
> >
> > http://lkml.org/lkml/2009/8/12/173
>
> What was happening in that case was that the rcu_dereference()
> was being used in a workqueue item. The role of rcu_read_lock()
> was taken on be the start of execution of the workqueue item, of
> rcu_read_unlock() by the end of execution of the workqueue item, and
> of synchronize_rcu() by flush_workqueue(). This does work, at least
> assuming that flush_workqueue() operates as advertised, which it appears
> to at first glance.
>
> The above code looks somewhat different, however -- I don't see
> handle_tx() being executed in the context of a work queue. Instead
> it appears to be in an interrupt handler.
> So what is the story? Using synchronize_irq() or some such?
>
> Thanx, Paul
No, there has been no change (I won't be able to use a mutex in an
interrupt handler, will I?). handle_tx is still called in the context
of a work queue: either from handle_tx_kick or from handle_tx_net which
are work queue items.
Can you ack this usage please?
--
MST
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Martin Devera @ 2009-11-04 11:51 UTC (permalink / raw)
To: Changli Gao; +Cc: Jarek Poplawski, Jamal Hadi Salim, netdev
In-Reply-To: <412e6f7f0911040330uc5ebbcekfef57854fce501f@mail.gmail.com>
Changli Gao wrote:
> On Wed, Nov 4, 2009 at 6:58 PM, Martin Devera <martin.devera@cdi.cz> wrote:
>> Hello,
>>
>> yes you are right. If I remember correctly, I tried to charge "rate"
>> tokens up to root in early versions (which makes some sense) but problem
>> is with many interior classes stuck at extreme mbuffer value, losing
>> responsivity.
>
> It is strange. Would you like describe it more clearly?
oops, forget it, I mixed it with different problem I remember :-)
The idea is to charge classe's tokens when it sends.
It always must be under ceil thus when it sends we can charge
ctokens (it must have some in order to be in CAN_SEND).
If it borrows, we don't charge it because doesn't send from
its own rate but rather from parent's rate. Thus charge
only parent. Because parent gave it rate as gift, free of charge,
it has it in addition to its own rate.
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Jarek Poplawski @ 2009-11-04 11:49 UTC (permalink / raw)
To: Changli Gao; +Cc: Jamal Hadi Salim, devik, netdev
In-Reply-To: <412e6f7f0911040321o22c536fdid078f6d2225a90a0@mail.gmail.com>
On Wed, Nov 04, 2009 at 07:21:48PM +0800, Changli Gao wrote:
...
> > cl->mbuffer is only to limit some extreme effects, so more of an
> > exception, not a main tool of rate control. (It really should be
> > useless most of the time if classes don't stop sending and aren't
> > deprived of their full rate for really long time.)
> >
>
> I don't think so. Although a class's tokens may be negative, but its
> ctokens may be positive. Charging its tokens is to prevent its cmode
> from being changed to HTB_CAN_SEND from HTB_CANT_SEND directly.
I think, you should really better show some tests proving your patch
is needed and doesn't affect a case I described, instead of trying to
discuss the meaninig of all HTB variables here.
Regards,
Jarek P.
^ permalink raw reply
* Re: HTB accuracy on 10GbE
From: Eric Dumazet @ 2009-11-04 11:31 UTC (permalink / raw)
To: Ryousei Takano
Cc: Stephen Hemminger, Patrick McHardy, Linux Netdev List,
takano-ryousei
In-Reply-To: <b30d1c3b0911040019q14a007d4lbd1b695db8884b27@mail.gmail.com>
Ryousei Takano a écrit :
>
> I tried iperf with 60 seconds samples. I got the almost same result.
>
> Here is the result:
> sender receiver
> 1.000 1.00 1.00
> 2.000 2.01 2.01
> 3.000 3.03 3.02
> 4.000 4.07 4.07
> 5.000 5.05 5.05
> 6.000 6.16 6.16
> 7.000 7.22 7.22
> 8.000 8.15 8.15
> 9.000 9.23 9.23
> 9.900 9.69 9.69
>
One thing to consider is the estimation error in qdisc_l2t(), rate table has only 256 slots
static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, unsigned int pktlen)
{
int slot = pktlen + rtab->rate.cell_align + rtab->rate.overhead;
if (slot < 0)
slot = 0;
slot >>= rtab->rate.cell_log;
if (slot > 255)
return (rtab->data[255]*(slot >> 8) + rtab->data[slot & 0xFF]);
return rtab->data[slot];
}
Maybe you can try changing class mtu to 40000 instead of 9000, and quantum to 60000 too
tc class add dev $DEV parent 1: classid 1:1 htb rate ${rate}mbit mtu 40000 quantum 60000
(because your tcp stack sends large buffers ( ~ 60000 bytes) as your NIC can offload tcp segmentation)
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Changli Gao @ 2009-11-04 11:30 UTC (permalink / raw)
To: Martin Devera; +Cc: Jarek Poplawski, Jamal Hadi Salim, netdev
In-Reply-To: <4AF15E73.30806@cdi.cz>
On Wed, Nov 4, 2009 at 6:58 PM, Martin Devera <martin.devera@cdi.cz> wrote:
>
> Hello,
>
> yes you are right. If I remember correctly, I tried to charge "rate"
> tokens up to root in early versions (which makes some sense) but problem
> is with many interior classes stuck at extreme mbuffer value, losing
> responsivity.
It is strange. Would you like describe it more clearly?
> Also it didn't played well with prio settings (class
> stalls).
Was it the cause?
> Bacause we don't charge "rate" now, some amount of burst can accumulate
> in each interior class and decrease fairness at the same prio level a bit.
> However from measurments I did, these bursts of unfairness are
> hidded by regular "bursting" of rate and ceil token buckets.
>
One case for unfairness.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Changli Gao @ 2009-11-04 11:21 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Jamal Hadi Salim, devik, netdev
In-Reply-To: <20091104104245.GB6224@ff.dom.local>
On Wed, Nov 4, 2009 at 6:42 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
> On Wed, Nov 04, 2009 at 05:16:42PM +0800, Changli Gao wrote:
> ...
>> It is just correct. You focus on 1 second fairness, while I focus on 2
>> seconds fairness.
>
> The whole example was very simplified, so it all would certainly
> differ in time and real sends, especially with an interaction of
> more classes. But, generally, main HTB algorithm seems to be quite
> well tested against various fair and unfair cases, starting from
> the author's examples:
> http://luxik.cdi.cz/~devik/qos/htb/manual/userg.htm
> so, I guess, this type of a bug would really show somewhere long time
> ago.
>
This documentation is old, and after the last update 5.5.2002, there
were still bugs: http://luxik.cdi.cz/~devik/qos/htb/ .
>> The token bucket and ctoken bucket both use cl->mbuffer to control
>> rate granularities. If we don't account token bucket when the
>> corresponding class in HTB_MAY_BORROW mode, the cl->mbuffer will
>> become useless.
>
> cl->mbuffer is only to limit some extreme effects, so more of an
> exception, not a main tool of rate control. (It really should be
> useless most of the time if classes don't stop sending and aren't
> deprived of their full rate for really long time.)
>
I don't think so. Although a class's tokens may be negative, but its
ctokens may be positive. Charging its tokens is to prevent its cmode
from being changed to HTB_CAN_SEND from HTB_CANT_SEND directly.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server
From: Andi Kleen @ 2009-11-04 11:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, virtualization, kvm, linux-kernel, mingo, linux-mm, akpm
In-Reply-To: <20091103172422.GD5591@redhat.com>
"Michael S. Tsirkin" <mst@redhat.com> writes:
Haven't really read the whole thing, just noticed something at a glance.
> +/* Expects to be always run from workqueue - which acts as
> + * read-size critical section for our kind of RCU. */
> +static void handle_tx(struct vhost_net *net)
> +{
> + struct vhost_virtqueue *vq = &net->dev.vqs[VHOST_NET_VQ_TX];
> + unsigned head, out, in, s;
> + struct msghdr msg = {
> + .msg_name = NULL,
> + .msg_namelen = 0,
> + .msg_control = NULL,
> + .msg_controllen = 0,
> + .msg_iov = vq->iov,
> + .msg_flags = MSG_DONTWAIT,
> + };
> + size_t len, total_len = 0;
> + int err, wmem;
> + size_t hdr_size;
> + struct socket *sock = rcu_dereference(vq->private_data);
> + if (!sock)
> + return;
> +
> + wmem = atomic_read(&sock->sk->sk_wmem_alloc);
> + if (wmem >= sock->sk->sk_sndbuf)
> + return;
> +
> + use_mm(net->dev.mm);
I haven't gone over all this code in detail, but that isolated reference count
use looks suspicious. What prevents the mm from going away before
you increment, if it's not the current one?
-Andi
--
ak@linux.intel.com -- Speaking for myself only.
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Martin Devera @ 2009-11-04 10:58 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Changli Gao, Jamal Hadi Salim, netdev
In-Reply-To: <20091104104245.GB6224@ff.dom.local>
Jarek Poplawski wrote:
> On Wed, Nov 04, 2009 at 05:16:42PM +0800, Changli Gao wrote:
> ...
>> It is just correct. You focus on 1 second fairness, while I focus on 2
>> seconds fairness.
>
> The whole example was very simplified, so it all would certainly
> differ in time and real sends, especially with an interaction of
> more classes. But, generally, main HTB algorithm seems to be quite
> well tested against various fair and unfair cases, starting from
> the author's examples:
> http://luxik.cdi.cz/~devik/qos/htb/manual/userg.htm
> so, I guess, this type of a bug would really show somewhere long time
> ago.
>
>> The token bucket and ctoken bucket both use cl->mbuffer to control
>> rate granularities. If we don't account token bucket when the
>> corresponding class in HTB_MAY_BORROW mode, the cl->mbuffer will
>> become useless.
>
> cl->mbuffer is only to limit some extreme effects, so more of an
> exception, not a main tool of rate control. (It really should be
> useless most of the time if classes don't stop sending and aren't
> deprived of their full rate for really long time.)
Hello,
yes you are right. If I remember correctly, I tried to charge "rate"
tokens up to root in early versions (which makes some sense) but problem
is with many interior classes stuck at extreme mbuffer value, losing
responsivity. Also it didn't played well with prio settings (class
stalls).
Bacause we don't charge "rate" now, some amount of burst can accumulate
in each interior class and decrease fairness at the same prio level a
bit. However from measurments I did, these bursts of unfairness are
hidded by regular "bursting" of rate and ceil token buckets.
devik
^ permalink raw reply
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Jarek Poplawski @ 2009-11-04 10:42 UTC (permalink / raw)
To: Changli Gao; +Cc: Jamal Hadi Salim, devik, netdev
In-Reply-To: <412e6f7f0911040116q6b25b705k83e5a45464698af1@mail.gmail.com>
On Wed, Nov 04, 2009 at 05:16:42PM +0800, Changli Gao wrote:
...
> It is just correct. You focus on 1 second fairness, while I focus on 2
> seconds fairness.
The whole example was very simplified, so it all would certainly
differ in time and real sends, especially with an interaction of
more classes. But, generally, main HTB algorithm seems to be quite
well tested against various fair and unfair cases, starting from
the author's examples:
http://luxik.cdi.cz/~devik/qos/htb/manual/userg.htm
so, I guess, this type of a bug would really show somewhere long time
ago.
> The token bucket and ctoken bucket both use cl->mbuffer to control
> rate granularities. If we don't account token bucket when the
> corresponding class in HTB_MAY_BORROW mode, the cl->mbuffer will
> become useless.
cl->mbuffer is only to limit some extreme effects, so more of an
exception, not a main tool of rate control. (It really should be
useless most of the time if classes don't stop sending and aren't
deprived of their full rate for really long time.)
Regards,
Jarek P.
^ permalink raw reply
* Re: [Patch] net: fix incorrect counting in __scm_destroy()
From: Eric Dumazet @ 2009-11-04 10:29 UTC (permalink / raw)
To: Amerigo Wang; +Cc: linux-kernel, netdev, David S. Miller
In-Reply-To: <20091104100717.4785.57149.sendpatchset@localhost.localdomain>
Amerigo Wang a écrit :
> It seems that in __scm_destroy() we forgot to decrease
> the ->count after fput(->fp[i]), this may cause some
> problem when we recursively call fput() again.
>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: David S. Miller <davem@davemloft.net>
>
> ---
> diff --git a/net/core/scm.c b/net/core/scm.c
> index b7ba91b..fa53219 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -120,8 +120,10 @@ void __scm_destroy(struct scm_cookie *scm)
> fpl = list_first_entry(&work_list, struct scm_fp_list, list);
>
> list_del(&fpl->list);
> - for (i=fpl->count-1; i>=0; i--)
> + for (i = fpl->count-1; i >= 0; i--) {
> fput(fpl->fp[i]);
> + fpl->count--;
> + }
> kfree(fpl);
> }
>
Hmm, your patch seems suspicious.
Are you fixing a real crash/bug, or is it something you discovered in a code review ?
Given we kfree(fpl) at the end of loop, we cannot recursively call __scm_destroy()
on same fpl, it would be a bug anyway ?
So you probably need something better, like testing fpl->list being not re-included
in current->scm_work_list before kfree() it
^ permalink raw reply
* Re: [PATCH] e1000: the power down when running ifdown command
From: Naohiro Ooiwa @ 2009-11-04 10:27 UTC (permalink / raw)
To: Jeff Kirsher
Cc: jesse.brandeburg, peter.p.waskiewicz.jr, john.ronciak, davem,
Andrew Morton, netdev, svaidy, e1000-devel
In-Reply-To: <9929d2390911031337p6115fef0wc1b94e5141ead341@mail.gmail.com>
Jeff Kirsher wrote:
> 2009/11/3 Naohiro Ooiwa <nooiwa@miraclelinux.com>:
>> Jeff Kirsher wrote:
>>> 2009/10/31 Naohiro Ooiwa <nooiwa@miraclelinux.com>:
>>>
>>> I have added this patch to my tree for testing. This patch requires a
>>> fair amount of regression testing, so once its passed testing I will
>>> push the patch to David/netdev.
>> I appreciate the marge your tree.
>> If there is anything I can do, please let me know.
>>
>> And I know this patch is good for e100 driver too.
>> I would really like to create patch for it.
>> How do you think about e100 driver.
>>
>>
>> Thanks,
>> Naohiro Ooiwa
>>
>
> Patches are always welcome (referring to a e100 patch).
I am happy that you should say that.
I will try to create a patch for e100, e1000e, igb and ixgbe.
Before that, I should fix the following problems.
> As far as the e1000 patch goes, it has a number of issues which were
> found in testing. Here are just a few problems we saw:
> 1. ethtool -t - crashes the system
> 2. ethtool eth0 - always shows link/speed as 1000/Full even when there
> is no cable
> 3. ethtool -s eth0 autoneg on/off - system hang. Sometimes a copper
> interface will show up as fiber after this.
> 4. ethtool -d/-S/-g etc - will corrupt the stats of the interface
> while doing ifup/down
Thank you for your tests.
Oh, My patch is full of problem.
The rest is my work.
I will resend the patch after test of all ethtool's options.
At that time, I will tell you contents of my tests.
And I sad WOL works on my patch in previous mail.
But WOL doesn't work. Sorry, I wasn't myself.
I will fix it too.
>
> So it appears that more needs to be done to the driver to get this to
> work as expected.
>
> NAK
>
^ permalink raw reply
* Re: [PATCH] e1000: the power down when running ifdown command
From: Naohiro Ooiwa @ 2009-11-04 10:23 UTC (permalink / raw)
To: Stephen Hemminger
Cc: jeffrey.t.kirsher, jesse.brandeburg, peter.p.waskiewicz.jr,
john.ronciak, davem, Andrew Morton, netdev, svaidy, e1000-devel
In-Reply-To: <4AEE35D2.4000503@miraclelinux.com>
Naohiro Ooiwa wrote:
> Stephen Hemminger wrote:
>> On Sat, 31 Oct 2009 18:39:52 +0900
>> Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote:
>>
>> Does this work with Wake On Lan?
>
> Yes, it works WOL.
Sorry, I made a mistake.
The WOL doesn't work when my patch applied to kernel.
I wasn't myself.
I consider the WOL and I will resent the patch.
Thank you for your point.
thanks,
Naohiro Ooiwa
> But I worry that my test is enough.
>
> They are following:
> - simple data transmission after ifdown;ifup.
> - enable wol, ifup network device, system shutdown, and make sure wol work.
> - enable wol, ifdown network device, system shutdown, and make sure wol work.
> - while [ 0 ] ; do ifdown eth0 ; ifup eth0 ; done
> - while [ 0 ] ; do modprobe e1000 ; rmmod e1000 ; done
>
>
>> @@ -1265,6 +1287,7 @@ static int e1000_open(struct net_device *netdev)
>> goto err_setup_rx;
>>
>> e1000_power_up_phy(adapter);
>> + e1000_reset(adapter);
>>
>> adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
>> if ((hw->mng_cookie.status &
>
> This code fix problem that e1000 driver doesn't work to auto-negotiation
> once in a while.
> Maybe, the cause is that set state to D0 just before it.
> I found it by repeat of ifup and ifdown.
>
> If you find out other points and any necessary tests from my patch,
> please tell me. I will make sure them.
>
> Thanks you.
> Naohiro Ooiwa
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [Patch] net: fix incorrect counting in __scm_destroy()
From: Amerigo Wang @ 2009-11-04 10:04 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, Amerigo Wang, David S. Miller
It seems that in __scm_destroy() we forgot to decrease
the ->count after fput(->fp[i]), this may cause some
problem when we recursively call fput() again.
Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: David S. Miller <davem@davemloft.net>
---
diff --git a/net/core/scm.c b/net/core/scm.c
index b7ba91b..fa53219 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -120,8 +120,10 @@ void __scm_destroy(struct scm_cookie *scm)
fpl = list_first_entry(&work_list, struct scm_fp_list, list);
list_del(&fpl->list);
- for (i=fpl->count-1; i>=0; i--)
+ for (i = fpl->count-1; i >= 0; i--) {
fput(fpl->fp[i]);
+ fpl->count--;
+ }
kfree(fpl);
}
^ permalink raw reply related
* Re: [PATCH] sch_htb.c consume the classes's tokens bellow the HTB_CAN_SEND level
From: Changli Gao @ 2009-11-04 9:16 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: Jamal Hadi Salim, devik, netdev
In-Reply-To: <20091104082808.GA6224@ff.dom.local>
On Wed, Nov 4, 2009 at 4:28 PM, Jarek Poplawski <jarkao2@gmail.com> wrote:
>> >
>> > If it's really necessary you should present some test case fixed by
>> > your patch, I guess.
>> >
>> > In the meantime let's consider what could be broken:
>> > class 1:1 (parent) rate 10 packets/sec
>> > class 1:2 rate 5 packets/sec ceil 10 packets/sec
>> > class 1:3 rate 5 packets/sec ceil 10 packets/sec
>> >
>> > class 1:2 doesn't use all its rate, and sends every other second
>> > (in even seconds)
>> > class 1:3 sends 10 packets during the first second, so with your
>> > patch it will use its tokens for 2 seconds
>> > class 1:2 uses its rate in the second second..., so class 1:1
>> > can't lend anything
>> > class 1:3 can only borrow, so it won't be able to send during
>> > this second anything
>> >
>> > So, the effect would be class 1:3 sending every odd second 10 packets
>> > while every even second - nothing...
>>
>> class 1:3 can send, as its parent rate is 10, but class 1:2 only uses
>> half of it, and class 1:1 is still in HTB_CAN_SEND mode.
>>
>> The result is, hasn't any difference with or without my patch :
>> class 1:1 sends 10 packets in odd seconds, and 5 packets in even seconds.
>
> I guess you meant class 1:3.
You are right. :)
> and there is a difference: it sends 5
> packets in even seconds only if it manages to borrow from 1:1, but
> it's not _guaranteed_ at all. In this particular case it's quite
> probable class 1:2 will send 10 packets in even seconds instead, or
> with some finer borrowing control it could be: class 1:2 8 packets,
> class 1:3 2 packets, as well.
It is just correct. You focus on 1 second fairness, while I focus on 2
seconds fairness.
>
>> class 1:2 sends 5 packets in even seconds.
>> class 1:1 (parent) sends 10 packets in every second.
>>
>> Let's think this case in another way: which class sends packets in
>> even seconds first, class 1:2 or class 1:3.
>> With my patch, as 1:3 in HTB_MAY_BORROW mode, and 1:2 in HTB_CAN_SEND
>> mode, so 1:2 sends all its 5 packets first.
>> Without my patch, as 1:2 and 1:3 are both in HTB_CAN_SEND mode, the
>> sequence is undetermined. In other word, 1:2 and 1:3 are treated
>> fairly, and it isn't fair for 1:2, because 1:2 sends nothing in odd
>> seconds, and has no deficit in rate as 1:3.
>
> The token bucket (cl->buffer) is just to account distinctly when a
> class is entitled to send and when it actually does send within its
> rate. The fairness is controlled by classes itself with HTB_CAN_SEND
> state. Using this bucket to account for borrowed sending deprives us
> of this precise information. "The fairness" would be controlled by
> priorities of borrowing instead (see above).
>
The token bucket and ctoken bucket both use cl->mbuffer to control
rate granularities. If we don't account token bucket when the
corresponding class in HTB_MAY_BORROW mode, the cl->mbuffer will
become useless.
--
Regards，
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [Bugme-new] [Bug 14427] New: ipv6 forward cause strange route
From: Pekka Savola @ 2009-11-04 8:01 UTC (permalink / raw)
To: YOSHIFUJI Hideaki
Cc: Andrew Morton, green, netdev, bugzilla-daemon, bugme-daemon,
davem
In-Reply-To: <4AF011A6.1070903@linux-ipv6.org>
On Tue, 3 Nov 2009, YOSHIFUJI Hideaki wrote:
>> RFC 2526: Reserved IPv6 Subnet Anycast Addresses
>> RFC 3627: Use of /127 Prefix Length Between Routers Considered Harmful
>
> I should say "Subnet-router anycast address" and
> RFC3513: Internet Protocol Version 6 (IPv6) Addressing
> Architecture".
FWIW, please note that commercial vendors haven't implemented this
very extensively, and there's also some recent activity arguing this
is not very useful:
http://tools.ietf.org/html/draft-kohno-ipv6-prefixlen-p2p-00
We'll likely get better idea how folks react to this during the next
week's IETF.
--
Pekka Savola "You each name yourselves king, yet the
Netcore Oy kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings
^ permalink raw reply
* Re: [announce] new rt2800 drivers for Ralink wireless & project tree
From: Ingo Molnar @ 2009-11-04 8:37 UTC (permalink / raw)
To: Ivo van Doorn
Cc: Bartlomiej Zolnierkiewicz, linux-wireless, linux-kernel, netdev,
Randy Dunlap, Luis Correia, John W. Linville, Johannes Berg,
Jarek Poplawski, Pekka Enberg, David Miller
In-Reply-To: <200911032200.04516.IvDoorn@gmail.com>
* Ivo van Doorn <ivdoorn@gmail.com> wrote:
> > MAINTAINERS: add rt2800 entry
>
> I see you decided to take over the maintainership? Doesn't that need
> the current maintainer to move away, or was this part of the "going
> over other peoples head" plan?
>
> [...]
>
> These are too much (and too big) patches for me to review at once,
> I'll look at them later.
Frankly, having read through the recent discussions related to the
rt2800pci/usb drivers, the subtle (and largely undeserved) group
violence and abuse you are inflicting on Bart is stomach-turning.
The non-working rt2800pci driver has been pending in your private tree
for how long, 1.5 _years_?
Look at the diffstat of Bart's driver:
15 files changed, 4036 insertions(+), 7158 deletions(-)
He reduced your 5.2 KLOC non-working driver into a 1.8 KLOC _working_
driver.
And _still_ your complaint about Bart's series is that he updated the
MAINTAINERS entry and added an entry for rt2800? Heck _sure_ he should
update it, he is the one doing the hard work of trying to bring it to
users, trying to clean up a messy driver space, trying to turn crap into
gold.
The thing is, if you dont have the time or interest to listen to and act
upon review feedback, be constructive about it and fix (obvious)
structural problems in your rt2800 code, you should just step aside and
let Bart maintain what he is apparently more capable of maintaining than
you are.
What you are doing here is a thinly veiled land-grab: you did a minimal
token driver for rt2800 that doesnt work, kept it in your private tree
for _1.5 years_, and the moment someone _else_ came along and did
something better and more functional in drivers/staging/, you discovered
your sudden interest for it and moved the crappy driver upstream at
lightning's speed (it is already in net-next AFAICS, despite negative
test and review feedback) - ignoring and throwing away all the work that
Bart has done.
Such behavior wouldnt fly in _any_ other Linux subsystem, but apparently
there is one set of rules for upstream kernel maintainers and then
there's another, different set of rules for upstream wireless driver
maintainers.
Really, you should listen to contrary opinion and _you_ should work
_hard_ to integrate Bart socially and open up your close circle of
wireless insiders instead of fighting his 'outsider' contributions every
which way. We dont care if people are rough, express displeasure and
show strong opinion about crappy code - but the moment you are
_excluding_ capable people and playing petty office politics (like you
are very clearly doing it with Bart here) everyone loses.
Guys, show some minimal amount of honesty, openness and critical
thinking please ...
Ingo
^ permalink raw reply
* Re: [PATCH] usbnet: Do not implement ethtool get_link() if link state is unknown
From: David Miller @ 2009-11-04 8:36 UTC (permalink / raw)
To: ben; +Cc: david-b, greg, jacmet, steve.glendinning, netdev
In-Reply-To: <1257304800.3136.452.camel@localhost>
From: Ben Hutchings <ben@decadent.org.uk>
Date: Wed, 04 Nov 2009 03:20:00 +0000
> I don't see how it's 'perfect' since it reports the link as up where it
> is really unknown. Still, this is a fairly minor bug.
That is the policy we enforce for all drivers, especially virtual
ones. If you can't tell, you specify that the link is up.
Otherwise automated tools like NetworkManager et al. will not even
attempt to bring do DHCP and bring the network device online.
^ 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