linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Fenghua Yu <fenghua.yu@intel.com>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Tony Luck <tony.luck@intel.com>,
	Randy Dunlap <rdunlap@infradead.org>,
	Xiaoyao Li <xiaoyao.li@intel.com>,
	Ravi V Shankar <ravi.v.shankar@intel.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>, x86 <x86@kernel.org>,
	Fenghua Yu <fenghua.yu@intel.com>
Subject: Re: [PATCH v5 2/3] x86/bus_lock: Handle #DB for bus lock
Date: Fri, 19 Mar 2021 22:30:50 +0100	[thread overview]
Message-ID: <871rca6dbp.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20210313054910.2503968-3-fenghua.yu@intel.com>

On Sat, Mar 13 2021 at 05:49, Fenghua Yu wrote:
> Change Log:
> v5:
> Address all comments from Thomas:
> - Merge patch 2 and patch 3 into one patch so all "split_lock_detect="
>   options are processed in one patch.

What? I certainly did not request that. I said:

 "Why is this seperate and an all in one thing? patch 2/4 changes the
  parameter first and 3/4 adds a new option...."

which means we want documentation for patch 2 and documentation for
patch 3? 

The ratelimit thing is clearly an extra functionality on top of that
buslock muck.

Next time I write it out..

> +	if (sscanf(arg, "ratelimit:%d", &ratelimit) == 1 && ratelimit > 0) {
> +		bld_ratelimit = ratelimit;

So any rate up to INTMAX/s is valid here, right?

> +	case sld_ratelimit:
> +		/* Enforce no more than bld_ratelimit bus locks/sec. */
> +		while (!__ratelimit(&get_current_user()->bld_ratelimit))
> +			msleep(1000 / bld_ratelimit);

which is cute because msleep() will always sleep until the next jiffie
increment happens.

What's not so cute here is the fact that get_current_user() takes a
reference on current's UID on every invocation, but nothing ever calls
free_uid(). I missed that last time over the way more obvious HZ division.

> +++ b/kernel/user.c
> @@ -103,6 +103,9 @@ struct user_struct root_user = {
>  	.locked_shm     = 0,
>  	.uid		= GLOBAL_ROOT_UID,
>  	.ratelimit	= RATELIMIT_STATE_INIT(root_user.ratelimit, 0, 0),
> +#ifdef CONFIG_CPU_SUP_INTEL
> +	.bld_ratelimit	= RATELIMIT_STATE_INIT(root_user.bld_ratelimit, 0, 0),
> +#endif
>  };
>  
>  /*
> @@ -172,6 +175,11 @@ void free_uid(struct user_struct *up)
>  		free_user(up, flags);
>  }
>  
> +#ifdef CONFIG_CPU_SUP_INTEL
> +/* Some Intel CPUs may set this for rate-limited bus locks. */
> +int bld_ratelimit;
> +#endif

Of course this variable is still required to be in the core kernel code
because?

While you decided to munge this all together, you obviously ignored the
following review comment:

  "It also lacks the information that the ratelimiting is per UID
   and not per task and why this was chosen to be per UID..."

There is still no reasoning neither in the changelog nor in the cover
letter nor in a reply to my review.

So let me repeat my question and make it more explicit:

  What is the justifucation for making this rate limit per UID and not
  per task, per process or systemwide?

>  struct user_struct *alloc_uid(kuid_t uid)
>  {
>  	struct hlist_head *hashent = uidhashentry(uid);
> @@ -190,6 +198,11 @@ struct user_struct *alloc_uid(kuid_t uid)
>  		refcount_set(&new->__count, 1);
>  		ratelimit_state_init(&new->ratelimit, HZ, 100);
>  		ratelimit_set_flags(&new->ratelimit, RATELIMIT_MSG_ON_RELEASE);
> +#ifdef CONFIG_CPU_SUP_INTEL
> +		ratelimit_state_init(&new->bld_ratelimit, HZ, bld_ratelimit);
> +		ratelimit_set_flags(&new->bld_ratelimit,
> +				    RATELIMIT_MSG_ON_RELEASE);
> +#endif

If this has a proper justification for being per user and having to add
40 bytes per UID for something which is mostly unused then there are
definitely better ways to do that than slapping #ifdefs into
architecture agnostic core code.

So if you instead of munging the code patches had split the
documentation, then I could apply the first 3 patches and we would only
have to sort out the ratelimiting muck.

Thanks,

        tglx

  reply	other threads:[~2021-03-19 21:31 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-13  5:49 [PATCH v5 0/3] x86/bus_lock: Enable bus lock detection Fenghua Yu
2021-03-13  5:49 ` [PATCH v5 1/3] x86/cpufeatures: Enumerate #DB for " Fenghua Yu
2021-03-19 20:35   ` Thomas Gleixner
2021-03-19 21:00     ` Fenghua Yu
2021-03-13  5:49 ` [PATCH v5 2/3] x86/bus_lock: Handle #DB for bus lock Fenghua Yu
2021-03-19 21:30   ` Thomas Gleixner [this message]
2021-03-19 21:50     ` Luck, Tony
2021-03-20  1:01       ` Thomas Gleixner
2021-03-20 13:57         ` Thomas Gleixner
2021-04-03  0:50           ` Fenghua Yu
2021-03-19 22:19     ` Fenghua Yu
2021-03-20 12:42       ` Thomas Gleixner
2021-04-03  1:04         ` Fenghua Yu
2021-04-12  7:15           ` Thomas Gleixner
2021-04-13 23:40             ` Fenghua Yu
2021-04-14  9:41               ` Thomas Gleixner
2021-03-13  5:49 ` [PATCH v5 3/3] Documentation/admin-guide: Change doc for split_lock_detect parameter Fenghua Yu
2021-03-19 21:35   ` Thomas Gleixner

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=871rca6dbp.fsf@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=bp@alien8.de \
    --cc=fenghua.yu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=rdunlap@infradead.org \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    --cc=xiaoyao.li@intel.com \
    /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).