LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Morton <akpm@linux-foundation.org>
To: Steven Hawkes <fsh016@ftw.mot.com>
Cc: davem@davemloft.net, joe@perches.com,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: printk_ratelimit and net_ratelimit conflict and tunable behavior
Date: Mon, 25 Feb 2008 15:45:21 -0800	[thread overview]
Message-ID: <20080225154521.e119cbb6.akpm@linux-foundation.org> (raw)
In-Reply-To: <200802252036.m1PKaeMY012587@ftw.mot.com>

On Mon, 25 Feb 2008 14:36:40 -0600 Steven Hawkes <fsh016@ftw.mot.com> wrote:

> From: Steve Hawkes <steve.hawkes@motorola.com>
> 
> The printk_ratelimit() and net_ratelimit() functions each have their own
> tunable parameters to control their respective rate limiting feature, but
> they share common state variables, preventing independent tuning of the
> parameters from working correctly. Also, changes to rate limiting tunable
> parameters do not always take effect properly since state is not recomputed
> when changes occur. For example, if ratelimit_burst is increased while rate
> limiting is occurring, the change won't take full effect until at least
> enough time between messages occurs so that the toks value reaches
> ratelimit_burst * ratelimit_jiffies. This can result in messages being
> suppressed when they should be allowed.
> 
> Implement independent state for printk_ratelimit() and net_ratelimit(), and
> update state when tunables are changed.
> 

This patch causes a large and nasty reject.

> ---
> --- linux-2.6.24/include/linux/kernel.h	2008-01-24 16:58:37.000000000 -0600
> +++ linux-2.6.24-printk_ratelimit/include/linux/kernel.h	2008-02-21 11:20:41.751197312 -0600

Probably because you patched 2.6.24.  We're developing 2.6.25 now, and the
difference between the two is very large inded.  Please raise patches
against Linus's latest tree?

There are other patches pending against printk.c (in -mm and in git-sched)
but afacit they won't collide.

> @@ -196,8 +196,19 @@ static inline int log_buf_copy(char *des
>  
>  unsigned long int_sqrt(unsigned long);
>  
> +struct printk_ratelimit_state
> +{

Please do

struct printk_ratelimit_state {

> +	unsigned long toks;
> +	unsigned long last_jiffies;
> +	int missed;
> +	int limit_jiffies;
> +	int limit_burst;
> +	char const *facility;
> +};

I find that the best-value comments one can add to kernel code are to the
members of structures.  If the reader understands what all the fields do, the
code becomes simple to follow.

> --- linux-2.6.24/net/core/utils.c	2008-01-24 16:58:37.000000000 -0600
> +++ linux-2.6.24-printk_ratelimit/net/core/utils.c	2008-02-21 11:03:44.644337698 -0600
> @@ -41,7 +41,16 @@ EXPORT_SYMBOL(net_msg_warn);
>   */
>  int net_ratelimit(void)
>  {
> -	return __printk_ratelimit(net_msg_cost, net_msg_burst);
> +	static struct printk_ratelimit_state limit_state = {
> +		.toks          = 10 * 5 * HZ,
> +		.last_jiffies  = 0,
> +		.missed        = 0,
> +		.limit_jiffies = 5 * HZ,
> +		.limit_burst   = 10,
> +		.facility      = "net"
> +	};
> +
> +	return __printk_ratelimit(net_msg_cost, net_msg_burst, &limit_state);

I don't get it.  There's one instance of limit_state, kernel-wide, and
__printk_ratelimit() modifies it.  What prevents one CPU's activities from
interfering with a second CPU's activities?


  reply	other threads:[~2008-02-25 23:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-25 20:36 Steven Hawkes
2008-02-25 23:45 ` Andrew Morton [this message]
2008-02-28 16:19   ` Hawkes Steve-FSH016
2008-02-28 18:39     ` Andrew Morton
  -- strict thread matches above, loose matches on Subject: below --
2008-02-28 16:10 Steven Hawkes
2008-02-19 21:30 Hawkes Steve-FSH016
2008-02-21  6:32 ` David Miller
2008-02-21  7:00   ` Joe Perches
2008-02-25 15:47     ` Hawkes Steve-FSH016
2008-02-25 20:02       ` David Miller
2008-02-25 20:13       ` Joe Perches
     [not found]         ` <7BFDACCD6948EF4D8FE8F4888A91596A016371FE@tx14exm60.ds.mot.com>
2008-02-26  0:04           ` Joe Perches

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=20080225154521.e119cbb6.akpm@linux-foundation.org \
    --to=akpm@linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=fsh016@ftw.mot.com \
    --cc=joe@perches.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: printk_ratelimit and net_ratelimit conflict and tunable behavior' \
    /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

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).