Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <ZSPayGSz6HQBp+3W@gmail.com> <202310092030430136422@zte.com.cn> <ZSP3cuEsgwWcIKRw@gmail.com>
In-Reply-To: <ZSP3cuEsgwWcIKRw@gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 9 Oct 2023 10:42:08 -0700
Message-ID: <CAJuCfpFabCn8gcuLV322RKC=xzVm0C+64HQP+CkFNJZ4VO42ZA@mail.gmail.com>
Subject: Re: [PATCH linux-next 2/3] sched/psi: Avoid update triggers and
 rtpoll_total when it is unnecessary
To:     Ingo Molnar <mingo@kernel.org>
Cc:     yang.yang29@zte.com.cn, peterz@infradead.org, hannes@cmpxchg.org,
        mingo@redhat.com, linux-kernel@vger.kernel.org,
        juri.lelli@redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 9, 2023 at 5:52=E2=80=AFAM Ingo Molnar <mingo@kernel.org> wrote=
:
>
>
> * yang.yang29@zte.com.cn <yang.yang29@zte.com.cn> wrote:
>
> > From: Yang Yang <yang.yang29@zte.com.cn>
> >
> > When psimon wakes up and there are no state changes for rtpoll_states,
> > it's unnecessary to update triggers and rtpoll_total because the pressu=
res
> > being monitored by user had not changed. This will help to slightly red=
uce
> > unnecessary computations of psi.
> >
> > And update group->rtpoll_next_update after called update_triggers() and
> > update rtpoll_total. This will prevent bugs if update_triggers() uses
> > group->rtpoll_next_update in the future, and it makes more sense
> > to set the next update time after we finished the current update.
>
> >       if (now >=3D group->rtpoll_next_update) {
> > -             update_triggers(group, now, &update_total, PSI_POLL);
> > -             group->rtpoll_next_update =3D now + group->rtpoll_min_per=
iod;
> > -             if (update_total)
> > +             if (changed_states & group->rtpoll_states) {
> > +                     update_triggers(group, now, &update_total, PSI_PO=
LL);
> >                       memcpy(group->rtpoll_total, group->total[PSI_POLL=
],
> >                                  sizeof(group->rtpoll_total));
> > +             }
> > +             group->rtpoll_next_update =3D now + group->rtpoll_min_per=
iod;
>
> So please also split out the second change into a separate patch as well,
> as it's an unrelated patch to the state-change optimization.

I think that the second part could have been done in the first patch
to place the "group->rtpoll_next_update =3D now +
group->rtpoll_min_period" line at the right place from the beginning.
Also when posting the next version please add the version number to
all the patch titles in the patchset, not only to the cover letter.
That helps with finding the latest version.
Thanks!

>
> We have a "one conceptual change per patch" rule for most things.
>
> Thanks,
>
>         Ingo