From: Phil Auld <pauld@redhat.com>
To: David Laight <David.Laight@aculab.com>
Cc: 'Benjamin Segall' <bsegall@google.com>, Hui Su <sh_def@163.com>,
"mingo@redhat.com" <mingo@redhat.com>,
"peterz@infradead.org" <peterz@infradead.org>,
"juri.lelli@redhat.com" <juri.lelli@redhat.com>,
"vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
"dietmar.eggemann@arm.com" <dietmar.eggemann@arm.com>,
"rostedt@goodmis.org" <rostedt@goodmis.org>,
"mgorman@suse.de" <mgorman@suse.de>,
"bristot@redhat.com" <bristot@redhat.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sched/fair: remove the spin_lock operations
Date: Mon, 2 Nov 2020 08:53:41 -0500 [thread overview]
Message-ID: <20201102135341.GA154641@lorien.usersys.redhat.com> (raw)
In-Reply-To: <22f99ee1d9b245c2a356d4d555b54e6a@AcuMS.aculab.com>
On Fri, Oct 30, 2020 at 10:16:29PM +0000 David Laight wrote:
> From: Benjamin Segall
> > Sent: 30 October 2020 18:48
> >
> > Hui Su <sh_def@163.com> writes:
> >
> > > Since 'ab93a4bc955b ("sched/fair: Remove
> > > distribute_running fromCFS bandwidth")',there is
> > > nothing to protect between raw_spin_lock_irqsave/store()
> > > in do_sched_cfs_slack_timer().
> > >
> > > So remove it.
> >
> > Reviewed-by: Ben Segall <bsegall@google.com>
> >
> > (I might nitpick the subject to be clear that it should be trivial
> > because the lock area is empty, or call them dead or something, but it's
> > not all that important)
>
> I don't know about this case, but a lock+unlock can be used
> to ensure that nothing else holds the lock when acquiring
> the lock requires another lock be held.
>
> So if the normal sequence is:
> lock(table)
> # lookup item
> lock(item)
> unlock(table)
> ....
> unlock(item)
>
> Then it can make sense to do:
> lock(table)
> lock(item)
> unlock(item)
> ....
> unlock(table)
>
> although that ought to deserve a comment.
>
Nah, this one used to be like this :
raw_spin_lock_irqsave(&cfs_b->lock, flags);
lsub_positive(&cfs_b->runtime, runtime);
cfs_b->distribute_running = 0;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
It's just a leftover. I agree that if it was there for some other
purpose that it would really need a comment. In this case, it's an
artifact of patch-based development I think.
Cheers,
Phil
> avid
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
--
next prev parent reply other threads:[~2020-11-02 13:55 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-30 14:46 [PATCH] sched/fair: remove the spin_lock operations Hui Su
2020-10-30 15:42 ` Phil Auld
2020-10-30 18:48 ` Benjamin Segall
2020-10-30 22:16 ` David Laight
2020-11-02 13:53 ` Phil Auld [this message]
2020-11-02 14:10 ` Hui Su
2020-11-11 8:23 ` [tip: sched/core] sched/fair: Remove superfluous lock section in do_sched_cfs_slack_timer() tip-bot2 for Hui Su
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=20201102135341.GA154641@lorien.usersys.redhat.com \
--to=pauld@redhat.com \
--cc=David.Laight@aculab.com \
--cc=bristot@redhat.com \
--cc=bsegall@google.com \
--cc=dietmar.eggemann@arm.com \
--cc=juri.lelli@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mgorman@suse.de \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=sh_def@163.com \
--cc=vincent.guittot@linaro.org \
/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).