Messages in this thread | | | From | Vincent Guittot <> | Date | Thu, 12 Dec 2019 09:05:21 +0100 | Subject | Re: [PATCH v2] sched/fair: Do not set skip buddy up the sched hierarchy |
| |
Hi Josh,
On Fri, 6 Dec 2019 at 23:13, Josh Don <joshdon@google.com> wrote: > > Hi Vincent, > > Thanks for taking a look. > > > There is a mismatch between the author Venkatesh Pallipadi and the > > signoff Josh Don > > If Venkatesh is the original author and you have then done some > > modifications, your both signed-off should be there > > Venkatesh no longer works at Google, so I don't have a way to get in > touch with him. Is my signed-off insufficient for this case?
Maybe you can add a Co-developed-by tag to reflect your additional changes I guess that as long as you agree with the DCO, it's ok : https://www.kernel.org/doc/html/v5.4/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
Ingo, Peter, what do you think ?
> > > On Thu, Dec 5, 2019 at 11:57 PM Vincent Guittot > <vincent.guittot@linaro.org> wrote: > > > > Hi Josh, > > > > On Wed, 4 Dec 2019 at 21:06, Josh Don <joshdon@google.com> wrote: > > > > > > From: Venkatesh Pallipadi <venki@google.com> > > > > > > Setting skip buddy all the way up the hierarchy does not play well > > > with intra-cgroup yield. One typical usecase of yield is when a > > > thread in a cgroup wants to yield CPU to another thread within the > > > same cgroup. For such a case, setting the skip buddy all the way up > > > the hierarchy is counter-productive, as that results in CPU being > > > yielded to a task in some other cgroup. > > > > > > So, limit the skip effect only to the task requesting it. > > > > > > Signed-off-by: Josh Don <joshdon@google.com> > > > > There is a mismatch between the author Venkatesh Pallipadi and the > > signoff Josh Don > > If Venkatesh is the original author and you have then done some > > modifications, your both signed-off should be there > > > > Apart from that, the change makes sense to me > > > > > --- > > > v2: Only clear skip buddy on the current cfs_rq > > > > > > kernel/sched/fair.c | 18 +++++++++++------- > > > 1 file changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > index 08a233e97a01..0b7a1958ad52 100644 > > > --- a/kernel/sched/fair.c > > > +++ b/kernel/sched/fair.c > > > @@ -4051,13 +4051,10 @@ static void __clear_buddies_next(struct sched_entity *se) > > > > > > static void __clear_buddies_skip(struct sched_entity *se) > > > { > > > - for_each_sched_entity(se) { > > > - struct cfs_rq *cfs_rq = cfs_rq_of(se); > > > - if (cfs_rq->skip != se) > > > - break; > > > + struct cfs_rq *cfs_rq = cfs_rq_of(se); > > > > > > + if (cfs_rq->skip == se) > > > cfs_rq->skip = NULL; > > > - } > > > } > > > > > > static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se) > > > @@ -6552,8 +6549,15 @@ static void set_next_buddy(struct sched_entity *se) > > > > > > static void set_skip_buddy(struct sched_entity *se) > > > { > > > - for_each_sched_entity(se) > > > - cfs_rq_of(se)->skip = se; > > > + /* > > > + * One typical usecase of yield is when a thread in a cgroup > > > + * wants to yield CPU to another thread within the same cgroup. > > > + * For such a case, setting the skip buddy all the way up the > > > + * hierarchy is counter-productive, as that results in CPU being > > > + * yielded to a task in some other cgroup. So, only set skip > > > + * for the task requesting it. > > > + */ > > > + cfs_rq_of(se)->skip = se; > > > } > > > > > > /* > > > -- > > > 2.24.0.393.g34dc348eaf-goog > > >
| |