Re: [PATCH v10 02/18] thread_info: Add update_thread_flag() helpers

From: "Alex Bennée" <alex.bennee@linaro.org>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Christoffer Dall <cdall@kernel.org>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Marc Zyngier <marc.zyngier@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Oleg Nesterov <oleg@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v10 02/18] thread_info: Add update_thread_flag() helpers
Date: Wed, 23 May 2018 15:35:31 +0100	[thread overview]
Message-ID: <87lgcatp70.fsf@linaro.org> (raw)
In-Reply-To: <20180523135732.GK13470@e103592.cambridge.arm.com>

Dave Martin <Dave.Martin@arm.com> writes:

> On Wed, May 23, 2018 at 02:46:52PM +0100, Alex Bennée wrote:
>>
>> Dave Martin <Dave.Martin@arm.com> writes:
>>
>> > There are a number of bits of code sprinkled around the kernel to
>> > set a thread flag if a certain condition is true, and clear it
>> > otherwise.
>> >
>> > To help make those call sites terser and less cumbersome, this
>> > patch adds a new family of thread flag manipulators
>> >
>> > 	update*_thread_flag([...,] flag, cond)
>> >
>> > which do the equivalent of:
>> >
>> > 	if (cond)
>> > 		set*_thread_flag([...,] flag);
>> > 	else
>> > 		clear*_thread_flag([...,] flag);
>> >
>> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
>> > Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
>> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>> > Acked-by: Catalin Marinas <catalin.marinas@arm.com>
>> > Cc: Ingo Molnar <mingo@redhat.com>
>> > Cc: Peter Zijlstra <peterz@infradead.org>
>> > Cc: Oleg Nesterov <oleg@redhat.com>
>> > ---
>> >  include/linux/sched.h       |  6 ++++++
>> >  include/linux/thread_info.h | 11 +++++++++++
>> >  2 files changed, 17 insertions(+)
>> >
>
> [...]
>
>> > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
>> > index cf2862b..8d8821b 100644
>> > --- a/include/linux/thread_info.h
>> > +++ b/include/linux/thread_info.h
>> > @@ -60,6 +60,15 @@ static inline void clear_ti_thread_flag(struct thread_info *ti, int flag)
>> >  	clear_bit(flag, (unsigned long *)&ti->flags);
>> >  }
>> >
>> > +static inline void update_ti_thread_flag(struct thread_info *ti, int flag,
>> > +					 bool value)
>> > +{
>> > +	if (value)
>> > +		set_ti_thread_flag(ti, flag);
>> > +	else
>> > +		clear_ti_thread_flag(ti, flag);
>> > +}
>> > +
>>
>> value does seem a bit of vanilla non-informative name for a condition
>> flag but whatever:
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> I guess, though I couldn't some up with an obviously better name.
>
> I support "condition" would have worked, but it's more verbose.

Well as you use cond in the text I think cond would also work as an
abbreviated variable name. But its a minor nit ;-)

>
> Thanks for the review
> ---Dave

--
Alex Bennée
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm