* RE: [PATCH 0/4][RFC] lro: Generic Large Receive Offload for TCP traffic
From: Leonid Grossman @ 2007-07-31 3:58 UTC (permalink / raw)
To: Andrew Gallatin, Linas Vepstas
Cc: Jan-Bernd Themann, netdev, Thomas Klein, Jeff Garzik,
Jan-Bernd Themann, linux-kernel, linux-ppc, Christoph Raisch,
Marcus Eder, Stefan Roscher, David Miller
In-Reply-To: <46AE232F.3080409@myri.com>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Andrew Gallatin
> Sent: Monday, July 30, 2007 10:43 AM
> To: Linas Vepstas
> Cc: Jan-Bernd Themann; netdev; Thomas Klein; Jeff Garzik; Jan-Bernd
> Themann; linux-kernel; linux-ppc; Christoph Raisch; Marcus Eder;
Stefan
> Roscher; David Miller
> Subject: Re: [PATCH 0/4][RFC] lro: Generic Large Receive Offload for
> TCP traffic
>
>
> Here is a quick reply before something more official can
> be written up:
>
> Linas Vepstas wrote:
>
> > -- what is LRO?
>
> Large Receive Offload
>
> > -- Basic principles of operation?
>
> LRO is analogous to a receive side version of TSO. The NIC (or
> driver) merges several consecutive segments from the same connection,
> fixing up checksums, etc. Rather than up to 45 separate 1500 byte
> frames (meaning up to 45 trips through the network stack), the driver
> merges them into one 65212 byte frame. It currently works only
> with TCP over IPv4.
>
> LRO was, AFAIK, first though of by Neterion. They had a paper about
> it at OLS2005.
> http://www.linuxinsight.com/files/ols2005/grossman-reprint.pdf
>
> > -- Can I use it in my driver?
>
> Yes, it can be used in any driver.
>
> > -- Does my hardware have to have some special feature before I can
> use it?
>
> No.
Ditto. LRO hw assists (or full LRO offload, meaning that the merge
happens in the ASIC but the merge criteria are set by the host) are
quite beneficial, especially as the number of LRO connections gets very
large (Neterion has IP around fw/hw LRO and will release full hw LRO
offload in the next ASIC), but as Andrew indicated sw-only LRO
implementation can be done for any NIC and provides good results -
especially for non-jumbo workloads.
>
> > -- What sort of performance improvement does it provide?
Throughput?
> > Latency? CPU usage? How does it affect DMA allocation? Does it
> > improve only a certain type of traffic (large/small packets,
> etc.)
>
> The benefit is directly proportional to the packet rate.
>
> See my reply to the previous RFC for performance information. The
> executive summary is that for the myri10ge 10GbE driver on low end
> hardware with 1500b frames, I've seen it increase throughput by a
> factor of nearly 2.5x, while at the same time reducing CPU utilization
> by 17%. The affect for jumbo frames is less dramatic, but still
> impressive (1.10x, 14% CPU reduction)
>
> You can achieve better speedups if your driver receives into
> high-order pages.
>
> > -- Example code? What's the API? How should my driver use it?
>
> The 3/4 in this patch showed an example of converting a driver
> to use LRO for skb based receive buffers. I'm working on
> a patch for myri10ge that shows how you would use it in a driver
> which receives into pages.
>
> Cheers,
>
> Drew
> -
> 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
* Re: [PATCH] [NET]: fix multicast list when cloning sockets
From: Arnaldo Carvalho de Melo @ 2007-07-31 3:00 UTC (permalink / raw)
To: David Miller; +Cc: fleitner, dlstevens, netdev
In-Reply-To: <20070730.190144.26963470.davem@davemloft.net>
On 7/30/07, David Miller <davem@davemloft.net> wrote:
> From: Flavio Leitner <fleitner@redhat.com>
> Date: Mon, 30 Jul 2007 13:04:48 -0300
>
> >
> > The sock_copy() function uses memcpy() to clone the socket
> > including the struct ip_mc_socklist *mc_list pointer.
> >
> > The ip_mc_drop_socket() function is called when socket is closed
> > to free these objects leaving the other sockets cloned from the
> > same master socket with invalid pointers.
> >
> > This patch sets mc_list of cloned socket to NULL.
> >
> > Signed-off-by: Flavio Leitner <fleitner@redhat.com>
>
> Allowing non-datagram sockets to end up with a non-NULL inet->mc_list
> in the first place is a bug.
>
> Multicast subscriptions cannot even be used with TCP and DCCP, which
> are the only two users of these connection oriented socket functions.
>
> The first thing that TCP and DCCP do, in fact, for input packet
> processing is drop the packet if it is not unicast.
>
> Therefore the fix really is for the inet layer to reject multicast
> subscription requests on sockets for which that absolutely does not
> make sense. There is no reason these functions in
> inet_connection_sock.c should need to be mindful of multicast
> state. :-)
Well, we can add a BUG_ON there then 8)
Flavio, take a look at do_ip_setsockopt in net/ipv4/ip_sockglue.c, in
the IP_{ADD,DROP}_MEMBERSHIP labels.
Don't forget IPV6 (net/ipv6/ipv6_sockglue.c)
- Arnaldo
^ permalink raw reply
* Re: [PATCH net-2.6 0/2] [TCP]: Fix bidirectional brokeness
From: David Miller @ 2007-07-31 2:54 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: netdev
In-Reply-To: <Pine.LNX.4.64.0707301947210.8788@kivilampi-30.cs.helsinki.fi>
From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 30 Jul 2007 20:16:54 +0300 (EEST)
> These will generate you some hassle when you rebase tcp-2.6. Btw, you
> forgot to push tcp-2.6 out last time though I could assume it's
> state... :-) In case you're going to now push it out, could you please
> drop "[TCP]: Remove num_acked>0 checks from cong.ctrl mods pkts_acked"
> from it as it seems to be out of place in tcp-2.6, I can resubmit it
> to net-2.6.24 when you open it (unless you want to put it to net-2.6
> directly as it's rather trivial one).
It has been in the back of my mind to take care of the tcp-2.6
tree and rebase it for you and others.
I'll take a stab at that soon hopefully.
Thanks for reminding me.
^ permalink raw reply
* Re: [PATCH net-2.6 2/2] [TCP]: Bidir flow must not disregard SACK blocks for lost marking
From: David Miller @ 2007-07-31 2:53 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: netdev
In-Reply-To: <Pine.LNX.4.64.0707302019110.8788@kivilampi-30.cs.helsinki.fi>
From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 30 Jul 2007 20:19:40 +0300 (EEST)
> It's possible that new SACK blocks that should trigger new LOST
> markings arrive with new data (which previously made is_dupack
> false). In addition, I think this fixes a case where we get
> a cumulative ACK with enough SACK blocks to trigger the fast
> recovery (is_dupack would be false there too).
>
> I'm not completely pleased with this solution because readability
> of the code is somewhat questionable as 'is_dupack' in SACK case
> is no longer about dupacks only but would mean something like
> 'lost_marker_work_todo' too... But because of Eifel stuff done
> in CA_Recovery, the FLAG_DATA_SACKED check cannot be placed to
> the if statement which seems attractive solution. Nevertheless,
> I didn't like adding another variable just for that either... :-)
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
I've applied this as well.
I don't mind the complex conditionals so much in loss
handling, they are almost inevitable. However I believe
they could be simplified as a lot of pieces of code ask
similar if not identical questions.
We could ask several of these things up-front, regardless
of path we will take (reno, DSACK, reorder, FRTO, etc.)
and pass the answers along in a bitmask. We do that to
some extent already with how we analyze the retransmit
queue at the beginning of ACK processing.
^ permalink raw reply
* Re: [TULIP] Need new maintainer
From: david @ 2007-07-31 2:52 UTC (permalink / raw)
To: Valerie Henson
Cc: Kyle McMartin, netdev, linux-kernel, tulip-users, Grant Grundler,
Jeff Garzik
In-Reply-To: <20070731015710.GA4196@rainbow>
On Mon, 30 Jul 2007, Valerie Henson wrote:
> On Mon, Jul 30, 2007 at 03:31:58PM -0400, Kyle McMartin wrote:
>> On Mon, Jul 30, 2007 at 01:04:13PM -0600, Valerie Henson wrote:
>>> The Tulip network driver needs a new maintainer! I no longer have
>>> time to maintain the Tulip network driver and I'm stepping down. Jeff
>>> Garzik would be happy to get volunteers.
>>>
>>
>> Since I already take care of a major consumer of these devices (parisc,
>> which pretty much all have tulip) I'm willing to take care of this.
>> Alternately, Grant is probably willing.
>
> And I coulda handed you a suitcase full of cards and I missed my
> chance!
>
> It's fine by me, although Jeff is the final arbiter.
>
> Thanks!
>
> -VAL
when the maintainership gets settled send me a message and I can send you
a couple of different cards (including a d-link 4-port card)
David Lang
^ permalink raw reply
* Re: [PATCH net-2.6 1/2] [TCP]: Fix ratehalving with bidirectional flows
From: David Miller @ 2007-07-31 2:49 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: netdev
In-Reply-To: <Pine.LNX.4.64.0707302017290.8788@kivilampi-30.cs.helsinki.fi>
From: "Ilpo_Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Mon, 30 Jul 2007 20:18:45 +0300 (EEST)
> Actually, the ratehalving seems to work too well, as cwnd is
> reduced on every second ACK even though the packets in flight
> remains unchanged. Recoveries in a bidirectional flows suffer
> quite badly because of this, both NewReno and SACK are affected.
>
> After this patch, rate halving is performed for ACK only if
> packets in flight was supposedly changed too.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
This fix looks great, and I see no potential problems with
Reno after doing a bit of auditing.
Applied and I'll push to -stable too.
^ permalink raw reply
* Re: fscked clock sources revisited
From: jamal @ 2007-07-31 2:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Robert.Olsson, shemminger, kaber
In-Reply-To: <20070730.183750.77058266.davem@davemloft.net>
On Mon, 2007-30-07 at 18:37 -0700, David Miller wrote:
> Relatively speaking, the acpi_pm numbers are at least consistent and
> about as much as so as jiffies. Jiffies numbers are also possibly
> better, at least in part, because of the decreased accuracy and errors
> propagating.
I am not sure jiffies gives innacurate results. It gives the best
performance on the dual xeon i tested on, all the time. Very consistent.
On its accuracy: I was able to validate it With pktgen generating
traffic and some external hardware tool capturing the interpacket gaps
etc.
> tsc acts as expected, since every time your cpu changes power
> management state (which it is going to do dynamically) the TSC
> rates change and thus the accuracy goes into outer-space.
Robert was saying he had even more bizare results with opteron given the
NUMA nature.
> There really isn't much that can be done by any of this. These issues
> exist because of hardware limitations, nobody bothered to build
> x86/x86_64 systems with a system wide TICK register that is both
> impervious to cpu frequence scaling and also cheap to access.
>
> So the above is what we basically have to live with :-)
That is a bummer. I am going to test with hpet when i get the chance
and perhaps turn off all the other sources if nothing good comes out; i
need my numbers ;->
cheers,
jamal
^ permalink raw reply
* Re: fscked clock sources revisited
From: Arjan van de Ven @ 2007-07-31 2:07 UTC (permalink / raw)
To: hadi
Cc: netdev, David Miller, Robert.Olsson, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <1185844980.5162.23.camel@localhost>
On Mon, 2007-07-30 at 21:23 -0400, jamal wrote:
> On Mon, 2007-30-07 at 21:17 -0400, jamal wrote:
>
> > Actually iirc, hpet is not even enabled in the
> > kernel -
>
> Sorry, i lied - the config file is on my laptop - it is enabled.
is it also on in the bios? (and if not, can you grab the patches to
force enable it instead?)
(they're in the -hrt patchkit and iirc also in -mm)
--
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org
^ permalink raw reply
* Re: fscked clock sources revisited
From: Arjan van de Ven @ 2007-07-31 2:03 UTC (permalink / raw)
To: David Miller; +Cc: hadi, netdev, Robert.Olsson, shemminger, kaber
In-Reply-To: <20070730.183750.77058266.davem@davemloft.net>
On Mon, 2007-07-30 at 18:37 -0700, David Miller wrote:
> There really isn't much that can be done by any of this. These issues
> exist because of hardware limitations, nobody bothered to build
> x86/x86_64 systems with a system wide TICK register that is both
> impervious to cpu frequence scaling and also cheap to access.
actually the tsc is clock speed invariant on pretty much all cpus these
days. The problem is that it stops in deeper idle states... (and that it
may be out of sync on an multi-socket system) not that it keeps changing
frequency ;(
(so it still sucks, just differently)
^ permalink raw reply
* Re: RFC: on [ab]use of skb->cb by VLAN code
From: David Miller @ 2007-07-31 2:06 UTC (permalink / raw)
To: hadi; +Cc: greearb, kaber, netdev, mcarlson
In-Reply-To: <1185847324.5162.31.camel@localhost>
From: jamal <hadi@cyberus.ca>
Date: Mon, 30 Jul 2007 22:02:04 -0400
> I came across the issue because i used cb in batching to store transient
> state which is used between qdisc dequeueing and hardware enqueueing
> (looked and smelled legit to me).
Right, dequeue->device should be OK and doesn't work because of
the VLAN issue.
^ permalink raw reply
* Re: [PATCH RFC]: napi_struct V4
From: David Miller @ 2007-07-31 2:03 UTC (permalink / raw)
To: rdreier; +Cc: netdev, shemminger, jgarzik, hadi, rusty
In-Reply-To: <adamyxdpbzg.fsf@cisco.com>
From: Roland Dreier <rdreier@cisco.com>
Date: Mon, 30 Jul 2007 19:01:55 -0700
> Are you saying that netif_rx_reschedule() itself is racy?
Nevermind, you're right.
I'll see if I can resurrect something akin to netif_rx_reschedule()
when I get back to the NAPI patches. Probably what I'll do is let
the ->poll() handler make sure to accumulate 'rx' work counts if
it loops again on netif_rx_reschedule() instead of dorking with
the quota and budget in that helper function.
^ permalink raw reply
* Re: RFC: on [ab]use of skb->cb by VLAN code
From: jamal @ 2007-07-31 2:02 UTC (permalink / raw)
To: David Miller; +Cc: greearb, kaber, netdev, mcarlson
In-Reply-To: <20070730.183337.27784025.davem@davemloft.net>
On Mon, 2007-30-07 at 18:33 -0700, David Miller wrote:
> I understand the concern, but how much qdisc stuff can possibly
> happen between those two ->hard_start_xmit() calls and do we
> want to support that in any way anyways?
>From a quick glance only netem seems to use it in the fast path (in a
legit way)
Theoretically, you could have many generations (i.e parents and
children, grandchildren etc) of netdevices stacked on top of each other
each with qdiscs. In a simple example: dont know how well these days
Vlans->bonding->somehardwarenetdevice works.
Redirect will could also result in a graph of unrelated netdevices (and
it is fair game to trample on cb anywhere along the path)
I came across the issue because i used cb in batching to store transient
state which is used between qdisc dequeueing and hardware enqueueing
(looked and smelled legit to me).
> The only alternative I see is to add more things to struct sk_buff
> and that's usually very unpopular :-)
I know ;-> Thats why i asked the question.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH RFC]: napi_struct V4
From: Roland Dreier @ 2007-07-31 2:01 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger, jgarzik, hadi, rusty
In-Reply-To: <20070730.154854.07455239.davem@davemloft.net>
> It is not a feature of these changes.
> These races exist in the current code, it is a bug fix.
Not sure I follow. What is the race in the current code? Are you
saying that netif_rx_reschedule() itself is racy? Otherwise the code
in ipoib looks OK to me.
- R.
^ permalink raw reply
* Re: [PATCH] [NET]: fix multicast list when cloning sockets
From: David Miller @ 2007-07-31 2:01 UTC (permalink / raw)
To: fleitner; +Cc: dlstevens, acme, netdev
In-Reply-To: <20070730160448.GA4002@redhat.com>
From: Flavio Leitner <fleitner@redhat.com>
Date: Mon, 30 Jul 2007 13:04:48 -0300
>
> The sock_copy() function uses memcpy() to clone the socket
> including the struct ip_mc_socklist *mc_list pointer.
>
> The ip_mc_drop_socket() function is called when socket is closed
> to free these objects leaving the other sockets cloned from the
> same master socket with invalid pointers.
>
> This patch sets mc_list of cloned socket to NULL.
>
> Signed-off-by: Flavio Leitner <fleitner@redhat.com>
Allowing non-datagram sockets to end up with a non-NULL inet->mc_list
in the first place is a bug.
Multicast subscriptions cannot even be used with TCP and DCCP, which
are the only two users of these connection oriented socket functions.
The first thing that TCP and DCCP do, in fact, for input packet
processing is drop the packet if it is not unicast.
Therefore the fix really is for the inet layer to reject multicast
subscription requests on sockets for which that absolutely does not
make sense. There is no reason these functions in
inet_connection_sock.c should need to be mindful of multicast
state. :-)
^ permalink raw reply
* Re: [TULIP] Need new maintainer
From: Valerie Henson @ 2007-07-31 1:57 UTC (permalink / raw)
To: Kyle McMartin
Cc: netdev, linux-kernel, tulip-users, Grant Grundler, Jeff Garzik
In-Reply-To: <20070730193158.GA13808@fattire.cabal.ca>
On Mon, Jul 30, 2007 at 03:31:58PM -0400, Kyle McMartin wrote:
> On Mon, Jul 30, 2007 at 01:04:13PM -0600, Valerie Henson wrote:
> > The Tulip network driver needs a new maintainer! I no longer have
> > time to maintain the Tulip network driver and I'm stepping down. Jeff
> > Garzik would be happy to get volunteers.
> >
>
> Since I already take care of a major consumer of these devices (parisc,
> which pretty much all have tulip) I'm willing to take care of this.
> Alternately, Grant is probably willing.
And I coulda handed you a suitcase full of cards and I missed my
chance!
It's fine by me, although Jeff is the final arbiter.
Thanks!
-VAL
^ permalink raw reply
* Re: [PATCH 2.6.23-rc1] PPPOL2TP: Add CONFIG_INET Kconfig dependency
From: David Miller @ 2007-07-31 1:52 UTC (permalink / raw)
To: jchapman; +Cc: netdev, toralf.foerster
In-Reply-To: <200707301205.l6UC5dmR026629@quickie.katalix.com>
From: James Chapman <jchapman@katalix.com>
Date: Mon, 30 Jul 2007 13:05:39 +0100
> [PPPOL2TP]: Add CONFIG_INET Kconfig dependency.
>
> PPPOL2TP uses UDP so it obviously depends on CONFIG_INET.
>
> Signed-off-by: James Chapman <jchapman@katalix.com>
Fair enough, applied, thanks James.
^ permalink raw reply
* [PATCH]: Fix sk_buff page offsets and lengths.
From: David Miller @ 2007-07-31 1:50 UTC (permalink / raw)
To: netdev; +Cc: sfr, shemminger
Stephen Rothwell pointed out to me that the skb_frag_struct
is broken on platforms using 64K or larger page sizes, it
even generates warnings when (for example) the myri10ge driver
tries to assign PAGE_SIZE into frag->size.
I've thus increased page offset and size to __u32 in the patch below.
I made this change much to even my own chagrin, but this is the
most direct fix and the ifdefs we could put here are both ugly
and also not something that we do with struct scatterlist so
no reason to do it in a place like this.
Actually, the cost on 64-bit is zero because there existed 4 bytes of
alignment padding for skb_frag_struct because of the page pointer.
On 32-bit the cost is up to 64-bytes :-/
Stephen, this opens up the doors a bit for the scatterlist work
you wanted to do in sk_buff.
commit 051c14dbc588590e0a165dda0305c7c1b9ce7fb0
Author: David S. Miller <davem@sunset.davemloft.net>
Date: Mon Jul 30 18:47:03 2007 -0700
[NET]: Page offsets and lengths need to be __u32.
Based upon a report from Stephen Rothwell.
Signed-off-by: David S. Miller <davem@davemloft.net>
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ce25643..93c27f7 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -134,8 +134,8 @@ typedef struct skb_frag_struct skb_frag_t;
struct skb_frag_struct {
struct page *page;
- __u16 page_offset;
- __u16 size;
+ __u32 page_offset;
+ __u32 size;
};
/* This data is invariant across clones and lives at
^ permalink raw reply related
* Re: fscked clock sources revisited
From: David Miller @ 2007-07-31 1:37 UTC (permalink / raw)
To: hadi; +Cc: netdev, Robert.Olsson, shemminger, kaber
In-Reply-To: <1185844239.5162.17.camel@localhost>
From: jamal <hadi@cyberus.ca>
Date: Mon, 30 Jul 2007 21:10:39 -0400
> acpi_pm: 108, 110, 111, 91, 108
> tsc: 143, 108, 161, 129, 108
> jiffies: 132, 138, 132, 146, 150
>
> jiffies produces better results than tsc which produces better results
> than acpi_pm.
Relatively speaking, the acpi_pm numbers are at least consistent and
about as much as so as jiffies. Jiffies numbers are also possibly
better, at least in part, because of the decreased accuracy and errors
propagating.
tsc acts as expected, since every time your cpu changes power
management state (which it is going to do dynamically) the TSC
rates change and thus the accuracy goes into outer-space.
There really isn't much that can be done by any of this. These issues
exist because of hardware limitations, nobody bothered to build
x86/x86_64 systems with a system wide TICK register that is both
impervious to cpu frequence scaling and also cheap to access.
So the above is what we basically have to live with :-)
^ permalink raw reply
* Re: RFC: on [ab]use of skb->cb by VLAN code
From: David Miller @ 2007-07-31 1:33 UTC (permalink / raw)
To: hadi; +Cc: greearb, kaber, netdev, mcarlson
In-Reply-To: <1185843556.5162.8.camel@localhost>
From: jamal <hadi@cyberus.ca>
Date: Mon, 30 Jul 2007 20:59:16 -0400
> This seems like a bad use since there can be a lot of things between
> a real hardware driver and something that sets a vlan tag (qdiscs come
> to mind).
I understand the concern, but how much qdisc stuff can possibly
happen between those two ->hard_start_xmit() calls and do we
want to support that in any way anyways?
The only alternative I see is to add more things to struct sk_buff
and that's usually very unpopular :-)
^ permalink raw reply
* Re: [PATCH 1/7] Preparatory refactoring part 1.
From: Corey Hickey @ 2007-07-31 1:26 UTC (permalink / raw)
To: Patrick McHardy; +Cc: netdev
In-Reply-To: <46ADECC9.7010303@trash.net>
Patrick McHardy wrote:
>> -static int
>> -sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> +static void sfq_q_enqueue(struct sk_buff *skb, struct sfq_sched_data *q, unsigned int end)
>
>
> Please make sure to break at 80 chars and to keep the style
> in this file consistent (newline before function name).
Ok. For what it's worth, though, most of the original functions in the
file don't have a newline before the function name. Omitting the newline
would thus make the new/changed functions more consistent with the
rest of the file. I don't have a preference either way, so unless you
change your mind I'll put the newline back in..
>> {
>> - struct sfq_sched_data *q = qdisc_priv(sch);
>> unsigned hash = sfq_hash(q, skb);
>> sfq_index x;
>>
>> @@ -256,8 +257,12 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> q->ht[hash] = x = q->dep[SFQ_DEPTH].next;
>> q->hash[x] = hash;
>> }
>> - sch->qstats.backlog += skb->len;
>
> Why not keep this instead of having both callers do it?
My idea was to have all the sfq_q_* functions operate on "struct
sfq_sched_data" and have no knowledge of the "struct Qdisc". I did this
in order to be able to use the new functions in sfq_change() when the
temporary sfq_sched_data doesn't have a parent Qdisc.
There's probably a better way, and I am of course open to suggestions,
but what I did made sense to me.
>> - __skb_queue_tail(&q->qs[x], skb);
>> +
>> + if (end == SFQ_TAIL)
>> + __skb_queue_tail(&q->qs[x], skb);
>> + else
>> + __skb_queue_head(&q->qs[x], skb);
>> +
>> sfq_inc(q, x);
>> if (q->qs[x].qlen == 1) { /* The flow is new */
>> if (q->tail == SFQ_DEPTH) { /* It is the first flow */
>> @@ -270,12 +275,21 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> q->tail = x;
>> }
>> }
>> +}
>> +
>> +static int
>> +sfq_enqueue(struct sk_buff *skb, struct Qdisc* sch)
>> +{
>> + struct sfq_sched_data *q = qdisc_priv(sch);
>
> newline please.
Ok.
>> + sfq_q_enqueue(skb, q, SFQ_TAIL);
>> + sch->qstats.backlog += skb->len;
>> if (++sch->q.qlen < q->limit-1) {
>> sch->bstats.bytes += skb->len;
>> sch->bstats.packets++;
>> return 0;
>> }
>>
>> + sch->qstats.drops++;
>
> sfq_drop already increments this.
You're right, of course. When I look at the original sfq_requeue(),
though, I see that same line right before sfq_drop(). That's probably
why it ended up in my patch. Is that a bug? The original sfq_enqueue()
doesn't have that line.
http://git.kernel.org/?p=linux/kernel/git/jgarzik/netdev-2.6.git;a=blob;f=net/sched/sch_sfq.c
line 314
>> sfq_drop(sch);
>> return NET_XMIT_CN;
>> }
>> @@ -284,28 +298,8 @@ static int
>> sfq_requeue(struct sk_buff *skb, struct Qdisc* sch)
>> {
>> struct sfq_sched_data *q = qdisc_priv(sch);
>
> newline please
Ok.
>> -static struct sk_buff *
>> -sfq_dequeue(struct Qdisc* sch)
>> +static struct sk_buff *sfq_q_dequeue(struct sfq_sched_data *q)
>
>
> Keep style consistent please.
Ok.
Thank you for the review. I'll address the rest of your comments when I
have time later.
-Corey
^ permalink raw reply
* Re: fscked clock sources revisited
From: jamal @ 2007-07-31 1:23 UTC (permalink / raw)
To: Arjan van de Ven
Cc: netdev, David Miller, Robert.Olsson, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <1185844629.5162.20.camel@localhost>
On Mon, 2007-30-07 at 21:17 -0400, jamal wrote:
> Actually iirc, hpet is not even enabled in the
> kernel -
Sorry, i lied - the config file is on my laptop - it is enabled.
cheers,
jamal
^ permalink raw reply
* Re: fscked clock sources revisited
From: jamal @ 2007-07-31 1:17 UTC (permalink / raw)
To: Arjan van de Ven
Cc: netdev, David Miller, Robert.Olsson, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <1185844218.2683.0.camel@laptopd505.fenrus.org>
On Mon, 2007-30-07 at 18:10 -0700, Arjan van de Ven wrote:
> can you make sure hpet is enabled as well?
Will do next opportunity. Actually iirc, hpet is not even enabled in the
kernel - What are you expecting to see?
cheers,
jamal
^ permalink raw reply
* Re: fscked clock sources revisited
From: Arjan van de Ven @ 2007-07-31 1:10 UTC (permalink / raw)
To: hadi
Cc: netdev, David Miller, Robert.Olsson, Stephen Hemminger,
Patrick McHardy
In-Reply-To: <1185844239.5162.17.camel@localhost>
On Mon, 2007-07-30 at 21:10 -0400, jamal wrote:
> Folks,
>
> I have posted this before but got no good response and i havent had time
> to chase it.
> While doing some batching tests with pktgen and then with a simple
> client server app with udp, it does appear that the clock source used
> matters. Here are some basic runs with plain vanilla 2.6.22-rc4 runs
> with udp. There are five runs per clock source and each result is in
> Mbps.
can you make sure hpet is enabled as well?
^ permalink raw reply
* fscked clock sources revisited
From: jamal @ 2007-07-31 1:10 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Robert.Olsson, Stephen Hemminger, Patrick McHardy
Folks,
I have posted this before but got no good response and i havent had time
to chase it.
While doing some batching tests with pktgen and then with a simple
client server app with udp, it does appear that the clock source used
matters. Here are some basic runs with plain vanilla 2.6.22-rc4 runs
with udp. There are five runs per clock source and each result is in
Mbps.
acpi_pm: 108, 110, 111, 91, 108
tsc: 143, 108, 161, 129, 108
jiffies: 132, 138, 132, 146, 150
jiffies produces better results than tsc which produces better results
than acpi_pm.
Second issue: I cant consistently get two reboots to always boot with
the same clock source.
Any ideas whats going on here? I havent tried with latest tree.
PS:- These results are very easy to validate with iperf or netperf.
cheers,
jamal
^ permalink raw reply
* Re: [2.6 patch] net/unix/af_unix.c: make code static
From: David Miller @ 2007-07-31 1:06 UTC (permalink / raw)
To: bunk; +Cc: mszeredi, netdev, linux-kernel
In-Reply-To: <20070729145852.GA16817@stusta.de>
From: Adrian Bunk <bunk@stusta.de>
Date: Sun, 29 Jul 2007 16:58:52 +0200
> The following code can now become static:
> - struct unix_socket_table
> - unix_table_lock
>
> Signed-off-by: Adrian Bunk <bunk@stusta.de>
Also applied, thanks a lot.
^ 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