Messages in this thread | | | Date | Fri, 19 Mar 2021 22:19:03 +0000 | From | Fenghua Yu <> | Subject | Re: [PATCH v5 2/3] x86/bus_lock: Handle #DB for bus lock |
| |
Hi, Thomas,
On Fri, Mar 19, 2021 at 10:30:50PM +0100, Thomas Gleixner wrote: > 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..
Sorry for misunderstanding your comments. I will split the document patch into two: one for patch 2 (warn and fatal) and one for patch 3 (ratelimit).
> > > + if (sscanf(arg, "ratelimit:%d", &ratelimit) == 1 && ratelimit > 0) { > > + bld_ratelimit = ratelimit; > > So any rate up to INTMAX/s is valid here, right?
Yes. I don't see smaller limitation than INTMX/s. Is that 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.
I will call free_uid().
> > > +++ 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?
Tony jut now answered the justification. If that's OK, I will add the answer in the commit message.
> > > 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.
If I split this whole patch set into two patch sets: 1. Three patches in the first patch set: the enumeration patch, the warn and fatal patch, and the documentation patch. 2. Two patches in the second patch set: the ratelimit patch and the documentation patch.
Then I will send the two patch sets separately, you will accept them one by one. Is that OK?
Or should I still send the 5 patches in one patch set so you will pick up the first 3 patches and then the next 2 patches separately?
Thanks.
-Fenghua
| |