netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jarek Poplawski <jarkao2@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: johannes@sipsolutions.net, tilman@imap.cc,
	hidave.darkstar@gmail.com, linux-kernel@vger.kernel.org,
	tglx@linutronix.de, linux-wireless@vger.kernel.org,
	linux-ppp@vger.kernel.org, netdev@vger.kernel.org,
	paulus@samba.org, mb@bu3sch.de, oliver@hartkopp.net
Subject: Re: [PATCH] net: Adjust softirq raising in __napi_schedule
Date: Thu, 22 Oct 2009 12:54:15 +0000	[thread overview]
Message-ID: <20091022125415.GA21699@ff.dom.local> (raw)
In-Reply-To: <20091022.042939.95166154.davem@davemloft.net>

On Thu, Oct 22, 2009 at 04:29:39AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Wed, 21 Oct 2009 23:39:47 +0200
> 
> > I'm not sure I can understand your question. This patch is mainly to
> > avoid using netif_rx()/netif_rx_ni() pair as a test of proper process
> > context handling; IMHO there're better tools for this (lockdep,
> > WARN_ON's).
> 
> Semantically I think your patch is correct, but I wonder about cost.
> 
> Something that is a simply per-cpu inline "or" operation is now a
> function call and potentially mispredicted branch inside of
> raise_softirq_irqoff().
> 
> And netif_rx() is indeed a fast path for tunnels and other users so
> this does matter.
> 
> I like having people call things in the correct context the function
> was built for, and thus we can avoiryd completely useless operations and
> tests as we can now in netif_rx().

I like it too, but in this particular case I'm not sure netif_rx()
functionality requires this kind of separation; it looks to me quite
similarly to e.g. tasklet_schedule(), the same for process or softirq
contexts.

> 
> Makaing things general purpose costs something, and it costs too much
> here for this critical routine, sorry.
> 
> I was just having a talk with Nick Piggin about these kinds of issues
> today, too few people care about these ever encrouching tiny pieces
> of bloat that slow the kernel down gradually over time, and I simply
> won't stand for it when I notice it :-)

I'm not sure we're saving in the right place. As a matter of fact,
whenever I look into kernel/ code I can't see this kind of
optimization. There is quite a lot of WARN_ON's and if's. These NOHZ
warnings simply show somebody's else debugging triggers far from
places where it all started and is quite accidental, while this
particular "bug" should've been printed immediately long time ago, if
we really cared.

Since I understand it's a question of taste, and it's not anything
critical, I'm quite OK with staying with the old way (except old
bugs, I hope ;-).

Jarek P.

  reply	other threads:[~2009-10-22 12:54 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20091011095217.GA2200@darkstar>
     [not found] ` <1255255735.4095.53.camel@johannes.local>
     [not found]   ` <4AD1BF06.3050103@phoenixsoftware.de>
     [not found]     ` <1255261251.4095.143.camel@johannes.local>
2009-10-12  8:28       ` NOHZ: local_softirq_pending 08 Tilman Schmidt
2009-10-12 10:32         ` David Miller
2009-10-12 11:25           ` Tilman Schmidt
2009-10-15 11:40             ` Jarek Poplawski
2009-10-15 17:53               ` Jarek Poplawski
2009-10-21 18:46                 ` Tilman Schmidt
2009-10-21 21:19                   ` [PATCH] net: Adjust softirq raising in __napi_schedule Jarek Poplawski
2009-10-21 21:25                     ` Johannes Berg
2009-10-21 21:37                       ` Tilman Schmidt
2009-10-21 21:39                       ` Jarek Poplawski
2009-10-22  8:27                         ` Johannes Berg
2009-10-23 14:39                           ` Tilman Schmidt
2009-10-23 14:46                             ` Johannes Berg
2009-10-26  7:41                               ` Jarek Poplawski
2009-10-26  7:44                                 ` Johannes Berg
2009-10-26  7:54                                   ` Jarek Poplawski
2009-10-26  7:58                                     ` Johannes Berg
2009-10-26  8:47                                       ` Tilman Schmidt
2009-10-26  8:56                                         ` Johannes Berg
2009-10-27  0:52                                           ` Tilman Schmidt
     [not found]                                             ` <4AE64441.7060008-ZTO5kqT2PaM@public.gmane.org>
2009-10-27  7:01                                               ` Johannes Berg
2009-10-22 11:29                         ` David Miller
2009-10-22 12:54                           ` Jarek Poplawski [this message]
2009-10-22 23:37                   ` NOHZ: local_softirq_pending 08 Tilman Schmidt
2009-10-23 13:34                     ` Johannes Berg
2009-10-23 14:27                       ` Tilman Schmidt
2009-10-23 14:31                         ` Johannes Berg
2009-10-23 16:33                           ` Tilman Schmidt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20091022125415.GA21699@ff.dom.local \
    --to=jarkao2@gmail.com \
    --cc=davem@davemloft.net \
    --cc=hidave.darkstar@gmail.com \
    --cc=johannes@sipsolutions.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-ppp@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=mb@bu3sch.de \
    --cc=netdev@vger.kernel.org \
    --cc=oliver@hartkopp.net \
    --cc=paulus@samba.org \
    --cc=tglx@linutronix.de \
    --cc=tilman@imap.cc \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).