Netdev List
 help / color / mirror / Atom feed
* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Ingo Molnar @ 2009-08-27  9:06 UTC (permalink / raw)
  To: Neil Horman
  Cc: Steven Rostedt, David Miller, fweisbec, billfink, netdev, brice,
	gallatin
In-Reply-To: <20090827011718.GA5315@localhost.localdomain>


* Neil Horman <nhorman@tuxdriver.com> wrote:

> > TRACE_EVENT can be a little harder to learn, because it is all 
> > MACRO magic, but once you understand them, you'll find that they 
> > are very easy.
> 
> Yeah, macro magic is an understatement.  But I'll have the 
> conversions done in the next few days, no worries.

Cool, thanks Neil!

[ And we tracing folks are rather fond of that macro abuse, so if 
  you can think of ways it could be made even more abusively 
  C-alike, we are all ears ;-) ]

	Ingo

^ permalink raw reply

* Re: [PATCH 1/2]: pkt_sched: Convert qdisc_watchdog to tasklet_hrtimer
From: Jarek Poplawski @ 2009-08-27  8:36 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, tglx
In-Reply-To: <20090821.170330.217935645.davem@davemloft.net>

On 22-08-2009 02:03, David Miller wrote:
> None of this stuff should execute in hw IRQ context, therefore
> use a tasklet_hrtimer so that it runs in softirq context.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
...
>  void qdisc_watchdog_cancel(struct qdisc_watchdog *wd)
>  {
> -	hrtimer_cancel(&wd->timer);
> +	tasklet_hrtimer_cancel(&wd->timer);
>  	wd->qdisc->flags &= ~TCQ_F_THROTTLED;
>  }
>  EXPORT_SYMBOL(qdisc_watchdog_cancel);

I've had a second look at it and I wonder why it's not reported, but
since qdisc_watchdog_cancel is run in qdisc_reset with BHs disabled
it seems there should be (at least) a warning triggered in
tasklet_kill on positive in_interrupt test (unless I miss something,
but can't test it now).

Jarek P.

^ permalink raw reply

* Re: [patch 1/5] [PATCH] qeth: HiperSockets SIGA retry support on CC=2.
From: Frank Blaschka @ 2009-08-27  8:05 UTC (permalink / raw)
  To: Bastian Blank, davem, netdev, linux-s390, Klaus-Dieter Wacker
In-Reply-To: <20090827070547.GA32106@wavehammer.waldi.eu.org>

Bastian Blank schrieb:
> On Wed, Aug 26, 2009 at 02:01:06PM +0200, frank.blaschka@de.ibm.com wrote:
>> Qeth HiperSockets support now retries sending of packets when the
>> IBM System z signals a temporary resource shortage (e.g. target
>> buffer full). The packet is enqueued into the device queue.
>> After 3 times of unsuccessful send the packet is dropped.
> 
> What is the reason for that? How large is the timeout for each retry?
> Usually the upper layers should already handle dropped packages fine and
> adding another retry below can produce retry multiplication.
>

HiperSocket is very different from usual network hardware. Microcode can
notify the driver the target of an xmit can not receive the packet because of
a target buffer full condition (this is the queue->sync_iqdio_error == 2 error
code). The buffer full condition is only for the target of the xmit not the
senders hardware. There is no timeout for the retry. The retry is more or less
immediately. If the target buffer full is only a temporary(short) condition we
can stay at a higher throughput level because TCP congestion does not kick in.

>> @@ -3178,6 +3203,25 @@ int qeth_do_send_packet_fast(struct qeth
>>  	atomic_set(&queue->state, QETH_OUT_Q_UNLOCKED);
>>  	qeth_fill_buffer(queue, buffer, skb, hdr, offset, hd_len);
>>  	qeth_flush_buffers(queue, index, 1);
>> +	if (queue->sync_iqdio_error == 2) {
> 
> Is this a magic constant?
> 
> Bastian
> 



^ permalink raw reply

* Re: [patch 1/5] [PATCH] qeth: HiperSockets SIGA retry support on CC=2.
From: Bastian Blank @ 2009-08-27  7:05 UTC (permalink / raw)
  To: frank.blaschka; +Cc: davem, netdev, linux-s390, Klaus-Dieter Wacker
In-Reply-To: <20090826120135.998746000@de.ibm.com>

On Wed, Aug 26, 2009 at 02:01:06PM +0200, frank.blaschka@de.ibm.com wrote:
> Qeth HiperSockets support now retries sending of packets when the
> IBM System z signals a temporary resource shortage (e.g. target
> buffer full). The packet is enqueued into the device queue.
> After 3 times of unsuccessful send the packet is dropped.

What is the reason for that? How large is the timeout for each retry?
Usually the upper layers should already handle dropped packages fine and
adding another retry below can produce retry multiplication.

> @@ -3178,6 +3203,25 @@ int qeth_do_send_packet_fast(struct qeth
>  	atomic_set(&queue->state, QETH_OUT_Q_UNLOCKED);
>  	qeth_fill_buffer(queue, buffer, skb, hdr, offset, hd_len);
>  	qeth_flush_buffers(queue, index, 1);
> +	if (queue->sync_iqdio_error == 2) {

Is this a magic constant?

Bastian

-- 
... The prejudices people feel about each other disappear when they get
to know each other.
		-- Kirk, "Elaan of Troyius", stardate 4372.5

^ permalink raw reply

* Re: [PATCH] r8169: Reduce looping in the interrupt handler.
From: Eric W. Biederman @ 2009-08-27  5:38 UTC (permalink / raw)
  To: Francois Romieu
  Cc: David Dillow, Michael Riepe, Michael Buesch, Rui Santos,
	Michael B??ker, linux-kernel, netdev
In-Reply-To: <20090827052423.GA1709@electric-eye.fr.zoreil.com>

Francois Romieu <romieu@fr.zoreil.com> writes:

> [...]
>> Right now I really don't have much more test bandwidth.  So I tried
>> for something simple that would address my problem without
>> fundamentally changing the already tested logic.
>
> Fair enough.
>
> [...]
>> Perhaps we should unconditionally ack everything after changing the 
>> interrupt mask  ? If that would prevent races it sounds like a simple
>> fix.
>
> I should be able to implement something right this evening, please hold on.

Sounds good.

Eric

^ permalink raw reply

* Re: [PATCH] r8169: Reduce looping in the interrupt handler.
From: Francois Romieu @ 2009-08-27  5:24 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: David Dillow, Michael Riepe, Michael Buesch, Rui Santos,
	Michael B??ker, linux-kernel, netdev
In-Reply-To: <m1hbvu45jq.fsf@fess.ebiederm.org>

[...]
> Right now I really don't have much more test bandwidth.  So I tried
> for something simple that would address my problem without
> fundamentally changing the already tested logic.

Fair enough.

[...]
> Perhaps we should unconditionally ack everything after changing the 
> interrupt mask  ? If that would prevent races it sounds like a simple
> fix.

I should be able to implement something right this evening, please hold on.

-- 
Ueimor

^ permalink raw reply

* [GIT]: Networking
From: David Miller @ 2009-08-27  3:50 UTC (permalink / raw)
  To: torvalds; +Cc: akpm, netdev, linux-kernel


Two fixes:

1) Fix smc91x build on SMP, from Alexey Dobriyan

2) Handle out-of-memory situations more correctly when trying
   to refill the RX ring buffers in virtio_net driver.  From Rusty
   Russell.  Based upon user reports.

Please pull, thanks a lot!

The following changes since commit f415c413f458837bd0c27086b79aca889f9435e4:
  Linus Torvalds (1):
        Merge git://git.kernel.org/.../davem/net-2.6

are available in the git repository at:

  master.kernel.org:/pub/scm/linux/kernel/git/davem/net-2.6.git master

Alexey Dobriyan (1):
      smc91x: fix compilation on SMP

Rusty Russell (1):
      virtio: net refill on out-of-memory

 drivers/net/smc91x.c     |    2 +-
 drivers/net/virtio_net.c |   61 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 47 insertions(+), 16 deletions(-)

^ permalink raw reply

* Award!!
From: Hyundai @ 2009-08-27  3:28 UTC (permalink / raw)
  To: hym.com

Congrats... you have won, confirm receipt by sending your name,address, age, phone number etc to (donnelld@tlen.pl) 


^ permalink raw reply

* RE: [PATCH 0/9] drivers/net/s2io.c: Cleanups
From: Ramkrishna Vepa @ 2009-08-27  2:45 UTC (permalink / raw)
  To: Joe Perches, David Miller
  Cc: netdev, Anil Murthy, Sreenivasa Honnur, Sivakumar Subramani,
	Rastapur Santosh, akpm, linux-next
In-Reply-To: <1251326878.13026.52.camel@Joe-Laptop.home>

Please give us a few days to review these patches. We should be done by
the end of this week.

Thanks,
Ram
> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> Sent: Wednesday, August 26, 2009 3:48 PM
> To: David Miller
> Cc: netdev@vger.kernel.org; Anil Murthy; Sreenivasa Honnur; Sivakumar
> Subramani; Rastapur Santosh; Ramkrishna Vepa;
akpm@linux-foundation.org;
> linux-next@vger.kernel.org
> Subject: Re: [PATCH 0/9] drivers/net/s2io.c: Cleanups
> 
> On Wed, 2009-08-26 at 15:33 -0700, David Miller wrote:
> > From: Joe Perches <joe@perches.com>
> > Date: Mon, 24 Aug 2009 20:29:39 -0700
> > > Just a few cleanups, compiled, untested.  No hardware.
> > >   s2io.c: Use const for strings
> > >   s2io.c: Shorten code line length by using intermediate pointers
> > >   s2io.c: Use calculated size in kmallocs
> > >   s2io.c: use kzalloc
> > >   s2io.c: Make more conforming to normal kernel style
> > >   s2io.c: convert printks to pr_<level>
> > >   s2io.c: fix spelling explaination
> > >   s2io.c: Standardize statistics accessors
> > >   s2io.c: Convert skipped nic->config.tx_cfg[i]. to tx_cfg->
> >
> > Since this is a pretty serious set of cleanups, I'd like to
> > let the S2IO driver folks have some time to look at this and
> > at least have a chance to ACK/NACK them.
> 
> No worries.  That's fine.  It's only cleanups.
> 
> I haven't heard from them though and they haven't
> submitted or signed a patch for s2io in over a year.
> 
> Hey!  Neterion people!  Is anybody home?

^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Neil Horman @ 2009-08-27  1:17 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, David Miller, fweisbec, billfink, netdev, brice,
	gallatin
In-Reply-To: <alpine.DEB.2.00.0908262021300.18923@gandalf.stny.rr.com>

On Wed, Aug 26, 2009 at 08:29:59PM -0400, Steven Rostedt wrote:
> 
> On Wed, 26 Aug 2009, Neil Horman wrote:
> > > 
> > I'm not sure how the addition of an ftrace module constitutes a change to the
> > tracing infrastructure, but whatever, yes, no biggy.  I've bugun modifying the
> > TRACE_EVENT that I added to export the data I need directly.  Should be pretty
> > straightforward.  Dave I'll have a patch up on netdev in a day or two after I
> > test it.  Steven, should this still just go to netdev with a cc to you?  I'd
> > like to avoid repeating the same confusion here a second time around if I can
> 
> Yes, please Cc myself, and Ingo on those changes. I see where the 
> confusion came. It is where the code changes. The code in kernel/trace is 
> considered ftrace internals (there's internal tracing upkeep that is 
> needed for all plugins). But with TRACE_EVENT, those can happen totally 
> inside a subsystem without touching any tracing directory. Those are 
> yours, and the TRACE_EVENT is just an API to the rest of the kernel. We 
> don't even care if you add a header to include/trace/events/ (if it 
> follows the standard format).
> 
> But by adding a plugin, it causes more work for us. The plugin types do 
> not get automated like TRACE_EVENTs and for binary readers like perf and 
> trace-cmd, we need to hand export the binary format for them.
> 
Understood, I'll keep that in mind in the future.

> > 
> > > > 
> > > > Ok, I'm rather tired of arguing.  Dave, I'll leave this in your hands.  The code
> > > > I wrote works fairly well in my view, and I feel like the review on it was both
> > > > positive and sufficent for inclusion.  But thats not my call, its yours.  I can
> > > > meet my own need with a raw TRACE_EVENT for now just as easily.  IF you feel
> > > > like the skb plugin should be pulled, please do so, and let me know.  All I ask
> > > > is that you keep the skb_copy_datagram_iovec TRACE_EVENT in place.  If you pull
> > > > the ftrace plugin, I'll submit a subsequent patch to agument the printing format
> > > > so that I can gather the numa allocation and consumption data directly there.
> > > 
> > > Yes, please keep the TRACE_EVENT (I think we can all agree on that ;-).
> > > 
> > Yes, that is rather centeral to what I'm monitoring :)
> > 
> > > You probably already read my previous email on the matter. Don't delete 
> > > your plugin patch until we get everything you need with TRACE_EVENT alone.
> > > 
> > Its ok, I should have the TRACE_EVENT modified to export this stuff directly by
> > tomorrow or friday anyway.  I really honestly just liked the ftrace interface
> > better, I found it a bit less confusing :)
> 
> Heh, because it was just a bit of cut and paste. But as Frederic said, 
> very much prone to errors. And it breaks the binary userspace readers. 
> Your new type did not get exported via trace_export.c.
> 
> TRACE_EVENT can be a little harder to learn, because it is all MACRO 
> magic, but once you understand them, you'll find that they are very easy.
> 
Yeah, macro magic is an understatement.  But I'll have the conversions done in
the next few days, no worries.

Thanks!
Neil

> Thanks!
> 
> -- Steve
> 
> 

^ permalink raw reply

* Re: [PATCH 7/7] MAINTAINERS: update information for sfc network driver
From: David Miller @ 2009-08-27  1:04 UTC (permalink / raw)
  To: joe; +Cc: bhutchings, netdev, linux-net-drivers
In-Reply-To: <1251312467.13026.22.camel@Joe-Laptop.home>

From: Joe Perches <joe@perches.com>
Date: Wed, 26 Aug 2009 11:47:47 -0700

> On Wed, 2009-08-26 at 19:19 +0100, Ben Hutchings wrote:
>> Update the current maintainers and add our email addresses.  Robert
>> Stonehouse has moved on to other projects.
>> 
>> Move the linux-net-drivers alias to the top since it should always
>> reach the current maintainers at Solarflare.  Add netdev as the
>> relevant mailing list.
> []
>> +M:	linux-net-drivers@solarflare.com (alias for current maintainers)
> 
> Hi Ben
> 
> This form: "M:	bare address (extra stuff)" doesn't work
> with the get_maintainers script.
> 
> Can you please use something like this instead?
> 
> M:	Solarflare linux maintainers <linux-net-drivers@solarflare.com>
> 
> Perhaps this diff below.

Joe's version seems based upon reasonable requests, so I've
applied his diff.

Thanks.

^ permalink raw reply

* Re: [PATCH 6/7] sfc: Improve reliability of RX queue flushing
From: David Miller @ 2009-08-27  1:04 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1251310693.27345.19.camel@achroite>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 26 Aug 2009 19:18:13 +0100

> Reconfiguring the port requires us to flush all DMA queues.  In
> repeated testing we have found that RX flushes would sometimes fail
> because the RX DMA engine was not properly isolated from the MACs.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

^ permalink raw reply

* Re: [PATCH 5/7] sfc: Work around XMAC bug causing packet loss with some peers
From: David Miller @ 2009-08-27  1:04 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1251310679.27345.17.camel@achroite>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 26 Aug 2009 19:17:59 +0100

> Received frames must be re-clocked by the local XGXS to the 156.25 MHz
> (DDR) clock of the XGMII.  If the remote clock is slightly faster this
> can reduce a minimum IPG of 64 bit-times (1 cycle) to 32 bit-times
> (half a cycle).  If the XMAC detects that a frame has reached the
> maximum RX frame length in the same cycle that it receives one of
> these reduced IPGs, it may miss the IPG, causing two valid frames to
> be treated as a single invalid frame (over-length with bad CRC).
> 
> We work around this by increasing the maximum RX frame length so that
> peers with matched MTU will not provoke this bug.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

^ permalink raw reply

* Re: [PATCH 4/7] sfc: QT2025C: Use hard reset only
From: David Miller @ 2009-08-27  1:04 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1251310639.27345.15.camel@achroite>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 26 Aug 2009 19:17:19 +0100

> At probe time, falcon_reset_hw() performs a hard reset of the PHY
> along with Falcon.  There is no need to perform a soft reset later,
> and any access to standard MDIO registers before the PHY firmware has
> booted can interrupt the boot process, making the port unusable.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

^ permalink raw reply

* Re: [PATCH 3/7] mdio: Check MDIO_STAT1_FAULT in mdio45_links_ok()
From: David Miller @ 2009-08-27  1:04 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1251310618.27345.13.camel@achroite>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 26 Aug 2009 19:16:58 +0100

> Some PHYs will report that the link is up even though there is a fault
> condition.  Therefore, check the fault flag too.  We must also read
> STAT2 to reset this flag.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

^ permalink raw reply

* Re: [PATCH 2/7] sfc: Do not reinitialise XAUI serdes before it has completed reset
From: David Miller @ 2009-08-27  1:04 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1251310606.27345.11.camel@achroite>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 26 Aug 2009 19:16:46 +0100

> falcon_reset_xaui() waits for XGXS reset to complete, but the XAUI
> serdes reset may take longer.  It needs to check both reset active
> bits.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

^ permalink raw reply

* Re: [PATCH 1/7] sfc: Fix ordering of device registration and initial netif_carrier_off()
From: David Miller @ 2009-08-27  1:03 UTC (permalink / raw)
  To: bhutchings; +Cc: netdev, linux-net-drivers
In-Reply-To: <1251310587.27345.9.camel@achroite>

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Wed, 26 Aug 2009 19:16:27 +0100

> We must call netif_carrier_off() after the device is registered, not
> before, to set the operational state and user-space IFF_RUNNING flag
> correctly.
> 
> Since we don't want observers to see an intermediate state, open-code
> register_netdev() and add efx_update_name() and netif_carrier_off()
> into the locked region.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied.

^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Christoph Hellwig @ 2009-08-27  0:35 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: David Miller, nhorman, rostedt, fweisbec, billfink, netdev, brice,
	gallatin
In-Reply-To: <20090826235826.GA430@elte.hu>

On Thu, Aug 27, 2009 at 01:58:26AM +0200, Ingo Molnar wrote:
> I'm sorry you got that impression, but you are a maintainer yourself 
> so you might perhaps understand it why sooner or later, if a 
> maintainer's review does not get acted upon, one has to insist on 
> clean patches in stronger terms.

Cool down a bit :)  While I totally agree with you on all the technical
bits here I think a slightly nicer attitude towars Neil and Dave would
help the cause a lot..


^ permalink raw reply

* Re: [patch 0/5] s390: qeth patches for 2.6.32
From: David Miller @ 2009-08-27  0:35 UTC (permalink / raw)
  To: frank.blaschka; +Cc: netdev, linux-s390
In-Reply-To: <20090826120105.727838000@de.ibm.com>

From: frank.blaschka@de.ibm.com
Date: Wed, 26 Aug 2009 14:01:05 +0200

> here are a couple of qeth patches for 2.6.32 (net-next).
> 
> shortlog:
> Klaus-Dieter Wacker (3)
> qeth: HiperSockets SIGA retry support on CC=2.
> qeth: Cleanup for cast-type determination.
> qeth: display service_level info only when valid.
> 
> Ursula Braun (1)
> qeth: display "undefined" value of sysfs-attribute "layer2"
> 
> Frank Blaschka (1)
> qeth: scheduling while atomic during ifconfig online sequence

Looks good, all applied to net-next-2.6, thanks!

^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Christoph Hellwig @ 2009-08-27  0:34 UTC (permalink / raw)
  To: Neil Horman
  Cc: Ingo Molnar, David Miller, rostedt, fweisbec, billfink, netdev,
	brice, gallatin
In-Reply-To: <20090826223922.GA25318@hmsreliant.think-freely.org>

On Wed, Aug 26, 2009 at 06:39:22PM -0400, Neil Horman wrote:
> Steven specifically told me to submit the patch to the subsystem maintainer that
> I'm adding tracepoints for, and the only feedback I got on it was his one
> question, the answer to which I assume satisfied him, due to that there was no
> subseuqent discussion.  I'm going to ignore your previous emails, because,
> despite the various advantages of just using plain TRACE_EVENTs because you
> provide the ftrace interface, and I found it useful.  Your observation is
> correct, I like it, and thats what I wanted to use, so I used it.  If you don't
> want people to use it, don't provide it.

Neil, this attitude is a perfect way to end up on a shitlist.  I think
there is a fair case to make you didn't know that the ftrace plugin was
wrong when you did, but now you do.  And btw, I completely agree with
Ingo here - the TRACE_EVENT stuff is extremly userful to get borader
pictures of what's going on.  E.g. the combination of my unfortunately
not yet included xfs tracer and blktrace allowed debugging quite a lot
of interesting issues.

So instead of playing jackass here listen to what is the right approach
for it and fix it up.  It'll help us all in the end.


Looking forward to the day when plain DECLARE_TRACE goes away so people
can't "accidentally" use it.

^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Steven Rostedt @ 2009-08-27  0:29 UTC (permalink / raw)
  To: Neil Horman
  Cc: Ingo Molnar, David Miller, fweisbec, billfink, netdev, brice,
	gallatin
In-Reply-To: <20090827001426.GA26475@hmsreliant.think-freely.org>


On Wed, 26 Aug 2009, Neil Horman wrote:
> > 
> I'm not sure how the addition of an ftrace module constitutes a change to the
> tracing infrastructure, but whatever, yes, no biggy.  I've bugun modifying the
> TRACE_EVENT that I added to export the data I need directly.  Should be pretty
> straightforward.  Dave I'll have a patch up on netdev in a day or two after I
> test it.  Steven, should this still just go to netdev with a cc to you?  I'd
> like to avoid repeating the same confusion here a second time around if I can

Yes, please Cc myself, and Ingo on those changes. I see where the 
confusion came. It is where the code changes. The code in kernel/trace is 
considered ftrace internals (there's internal tracing upkeep that is 
needed for all plugins). But with TRACE_EVENT, those can happen totally 
inside a subsystem without touching any tracing directory. Those are 
yours, and the TRACE_EVENT is just an API to the rest of the kernel. We 
don't even care if you add a header to include/trace/events/ (if it 
follows the standard format).

But by adding a plugin, it causes more work for us. The plugin types do 
not get automated like TRACE_EVENTs and for binary readers like perf and 
trace-cmd, we need to hand export the binary format for them.

> 
> > > 
> > > Ok, I'm rather tired of arguing.  Dave, I'll leave this in your hands.  The code
> > > I wrote works fairly well in my view, and I feel like the review on it was both
> > > positive and sufficent for inclusion.  But thats not my call, its yours.  I can
> > > meet my own need with a raw TRACE_EVENT for now just as easily.  IF you feel
> > > like the skb plugin should be pulled, please do so, and let me know.  All I ask
> > > is that you keep the skb_copy_datagram_iovec TRACE_EVENT in place.  If you pull
> > > the ftrace plugin, I'll submit a subsequent patch to agument the printing format
> > > so that I can gather the numa allocation and consumption data directly there.
> > 
> > Yes, please keep the TRACE_EVENT (I think we can all agree on that ;-).
> > 
> Yes, that is rather centeral to what I'm monitoring :)
> 
> > You probably already read my previous email on the matter. Don't delete 
> > your plugin patch until we get everything you need with TRACE_EVENT alone.
> > 
> Its ok, I should have the TRACE_EVENT modified to export this stuff directly by
> tomorrow or friday anyway.  I really honestly just liked the ftrace interface
> better, I found it a bit less confusing :)

Heh, because it was just a bit of cut and paste. But as Frederic said, 
very much prone to errors. And it breaks the binary userspace readers. 
Your new type did not get exported via trace_export.c.

TRACE_EVENT can be a little harder to learn, because it is all MACRO 
magic, but once you understand them, you'll find that they are very easy.

Thanks!

-- Steve


^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Neil Horman @ 2009-08-27  0:14 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Ingo Molnar, David Miller, fweisbec, billfink, netdev, brice,
	gallatin
In-Reply-To: <alpine.DEB.2.00.0908261913030.11291@gandalf.stny.rr.com>

On Wed, Aug 26, 2009 at 07:33:55PM -0400, Steven Rostedt wrote:
> 
> On Wed, 26 Aug 2009, Neil Horman wrote:
> 
> > On Wed, Aug 26, 2009 at 10:40:27PM +0200, Ingo Molnar wrote:
> > > 
> > > * Neil Horman <nhorman@tuxdriver.com> wrote:
> > > 
> > > > On Wed, Aug 26, 2009 at 09:48:35PM +0200, Ingo Molnar wrote:
> > > > > 
> > > > > * David Miller <davem@davemloft.net> wrote:
> > > > > 
> > > > > > From: Ingo Molnar <mingo@elte.hu>
> > > > > > Date: Wed, 26 Aug 2009 21:08:30 +0200
> > > > > > 
> > > > > > > Sigh, no. Please re-read the past discussions about this. 
> > > > > > > trace_skb_sources.c is a hack and should be converted to generic 
> > > > > > > tracepoints. Is there anything in it that cannot be expressed in 
> > > > > > > terms of TRACE_EVENT()?
> > > > > > 
> > > > > > Neil explained why he needed to implement it this way in his reply 
> > > > > > to Steven Rostedt.  I attach it here for your convenience.
> > > > > 
> > > > > thanks. The argument is invalid:
> > > > 
> > > > Just because you assert that doesn't make it so, Ingo.
> > > 
> > > I stand by that statement, the argument is invalid, for the many 
> > > reasons i outlined in my previous mails. (you'd have gotten those 
> > > same arguments had you submitted that patch to the folks who 
> > > maintain kernel/trace/)
> > > 
> 
> > Steven specifically told me to submit the patch to the subsystem maintainer that
> > I'm adding tracepoints for, and the only feedback I got on it was his one
> > question, the answer to which I assume satisfied him, due to that there was no
> > subseuqent discussion.  I'm going to ignore your previous emails, because,
> > despite the various advantages of just using plain TRACE_EVENTs because you
> > provide the ftrace interface, and I found it useful.  Your observation is
> > correct, I like it, and thats what I wanted to use, so I used it.  If you don't
> > want people to use it, don't provide it.
> 
> Actually, I suggested to submit it to the subsystem maintainer if there 
> was no changes to the tracing infrastructure. We may have just had a 
> misunderstanding there. No biggy.
> 
I'm not sure how the addition of an ftrace module constitutes a change to the
tracing infrastructure, but whatever, yes, no biggy.  I've bugun modifying the
TRACE_EVENT that I added to export the data I need directly.  Should be pretty
straightforward.  Dave I'll have a patch up on netdev in a day or two after I
test it.  Steven, should this still just go to netdev with a cc to you?  I'd
like to avoid repeating the same confusion here a second time around if I can

> > 
> > Ok, I'm rather tired of arguing.  Dave, I'll leave this in your hands.  The code
> > I wrote works fairly well in my view, and I feel like the review on it was both
> > positive and sufficent for inclusion.  But thats not my call, its yours.  I can
> > meet my own need with a raw TRACE_EVENT for now just as easily.  IF you feel
> > like the skb plugin should be pulled, please do so, and let me know.  All I ask
> > is that you keep the skb_copy_datagram_iovec TRACE_EVENT in place.  If you pull
> > the ftrace plugin, I'll submit a subsequent patch to agument the printing format
> > so that I can gather the numa allocation and consumption data directly there.
> 
> Yes, please keep the TRACE_EVENT (I think we can all agree on that ;-).
> 
Yes, that is rather centeral to what I'm monitoring :)

> You probably already read my previous email on the matter. Don't delete 
> your plugin patch until we get everything you need with TRACE_EVENT alone.
> 
Its ok, I should have the TRACE_EVENT modified to export this stuff directly by
tomorrow or friday anyway.  I really honestly just liked the ftrace interface
better, I found it a bit less confusing :)

Best
Neil

> Thanks,
> 
> -- Steve
> 
> 

^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Steven Rostedt @ 2009-08-27  0:05 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: David Miller, nhorman, fweisbec, billfink, netdev, brice,
	gallatin
In-Reply-To: <20090826235826.GA430@elte.hu>


On Thu, 27 Aug 2009, Ingo Molnar wrote:

> 
> * David Miller <davem@davemloft.net> wrote:
> 
> > From: Ingo Molnar <mingo@elte.hu>
> > Date: Thu, 27 Aug 2009 01:05:14 +0200
> > 
> > > And the thing is, we are not rigid about these things in the 
> > > tracing tree and if this was a good change we would not mind and 
> > > you'd have my Ack. The problem is that it's a crappy change and 
> > > that Neil is refusing to fix it. So please fix it,
> > 
> > Thankfully, Steven Rostedt gave a much more useful and reasonable 
> > response than you.
> 
> I'm sorry you got that impression, but you are a maintainer yourself 
> so you might perhaps understand it why sooner or later, if a 
> maintainer's review does not get acted upon, one has to insist on 
> clean patches in stronger terms.
> 
> Unfortunately you took away the "do not apply the patch" option from 
> me that could have avoided the stronger words and could have kept 
> this discussion more polite.

I feel somewhat at fault here. Neil did give me a heads up on his project, 
but his patches went out when I was getting ready for vacation and had 
other priorities at the time. I could have brought up these issues before 
Dave took them, and he may have taken them because I did not.

But this is all water under the bridge. Time to be more productive.

-- Steve


^ permalink raw reply

* Re: Receive side performance issue with multi-10-GigE and NUMA
From: Ingo Molnar @ 2009-08-26 23:58 UTC (permalink / raw)
  To: David Miller
  Cc: nhorman, rostedt, fweisbec, billfink, netdev, brice, gallatin
In-Reply-To: <20090826.160835.263085197.davem@davemloft.net>


* David Miller <davem@davemloft.net> wrote:

> From: Ingo Molnar <mingo@elte.hu>
> Date: Thu, 27 Aug 2009 01:05:14 +0200
> 
> > And the thing is, we are not rigid about these things in the 
> > tracing tree and if this was a good change we would not mind and 
> > you'd have my Ack. The problem is that it's a crappy change and 
> > that Neil is refusing to fix it. So please fix it,
> 
> Thankfully, Steven Rostedt gave a much more useful and reasonable 
> response than you.

I'm sorry you got that impression, but you are a maintainer yourself 
so you might perhaps understand it why sooner or later, if a 
maintainer's review does not get acted upon, one has to insist on 
clean patches in stronger terms.

Unfortunately you took away the "do not apply the patch" option from 
me that could have avoided the stronger words and could have kept 
this discussion more polite.

Thanks,

	Ingo

^ permalink raw reply

* Re: multicast routing and multiple interfaces with same IP
From: Octavian Purdila @ 2009-08-26 23:53 UTC (permalink / raw)
  To: Ilia K.; +Cc: netdev
In-Reply-To: <1b9338490908111626j55b49177q71b8a373b6e6381b@mail.gmail.com>

On Wednesday 12 August 2009 02:26:21 Ilia K. wrote:
> Hi All,
> When routing daemon wants to enable forwarding of multicast traffic it
> performs something like:
>
> 	struct vifctl vc = {
> 		.vifc_vifi  = 1,
> 		.vifc_flags = 0,
> 		.vifc_threshold = 1,
> 		.vifc_rate_limit = 0,
> 		.vifc_lcl_addr = ip, /* <--- ip address of physical interface, e.g. eth0
> */ .vifc_rmt_addr.s_addr = htonl(INADDR_ANY),
> 	  };
> 	setsockopt(fd, IPPROTO_IP, MRT_ADD_VIF, &vc, sizeof(vc));
>
> This leads (in the kernel) to call to vif_add() function call which
> search the (physical) device using assigned IP address:
> 	dev = ip_dev_find(net, vifc->vifc_lcl_addr.s_addr);
>
> It seems like API (struct vifctl) does not allow to specify an
> interface other way than using it's IP, and if there are more than a
> single interface with specified IP only the first one will be found
> (for example it makes problems when tunnel is configured using the
> same IP as underlying interface).
>
> Am I correct in identifying the problem?
> I can propose the attached patch against 2.6.30.4.
>

Hi Ilia,

I don't have context on multicast routing, but this caught my attention:

>@@ -61,11 +61,13 @@ 
> 	unsigned int vifc_rate_limit;	/* Rate limiter values (NI) */
> 	struct in_addr vifc_lcl_addr;	/* Our address */
> 	struct in_addr vifc_rmt_addr;	/* IPIP tunnel addr */
>+	int ifindex;			/* Local interface index */
> };
>

Wouldn't this break userspace ABI? 

Perhaps you could use a union between vifc_lcl_addr and vifc_ifindex, they seem 
to be exclusive.

tavi


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox