Re: [PATCH] irqchip/mips-gic: mark count and compare accessors notrace

Marcin Nowakowski
2017-06-08T13:35:13Z

Hi Marc,

On 08.06.2017 15:26, Marc Zyngier wrote:
> On Thu, Jun 08 2017 at 3:06:23 pm BST, Marcin Nowakowski <marcin.nowakowski@imgtec.com> wrote:
>> gic_read_count(), gic_write_compare() and gic_write_cpu_compare() are
>> often used in a sequence to update the compare register with a count
>> value increased by a small offset.
>> With small delta values used to update the compare register, the time to
>> update function trace for these operations may be longer than the update
>> timeout leading to update failure.
>>
>> Signed-off-by: Marcin Nowakowski <marcin.nowakowski@imgtec.com>
>> ---
>> drivers/irqchip/irq-mips-gic.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
>> index eb7fbe1..ecee073 100644
>> --- a/drivers/irqchip/irq-mips-gic.c
>> +++ b/drivers/irqchip/irq-mips-gic.c
>> @@ -140,7 +140,7 @@ static inline void gic_map_to_vpe(unsigned int intr, unsigned int vpe)
>> }
>>
>> #ifdef CONFIG_CLKSRC_MIPS_GIC
>> -u64 gic_read_count(void)
>> +notrace u64 gic_read_count(void)
>
> The attributes are usually placed between the return type and the
> function name.

OK, I'll change this.

>> {
>> unsigned int hi, hi2, lo;
>>
>> @@ -167,7 +167,7 @@ unsigned int gic_get_count_width(void)
>> return bits;
>> }
>>
>> -void gic_write_compare(u64 cnt)
>> +notrace void gic_write_compare(u64 cnt)
>> {
>> if (mips_cm_is64) {
>> gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_COMPARE), cnt);
>> @@ -179,7 +179,7 @@ void gic_write_compare(u64 cnt)
>> }
>> }
>>
>> -void gic_write_cpu_compare(u64 cnt, int cpu)
>> +notrace void gic_write_cpu_compare(u64 cnt, int cpu)
>> {
>> unsigned long flags;
>
> What guarantees do you have that some event (interrupt? frequency
> scaling?) won't delay these anyway, generating the same missed deadline?
> Shouldn't the code deal with these case and acknowledge that the
> deadline has already expired?

Well - there is no guarantee for that at the moment. One solution that
kernel provides (and that works in this scenario) is to enable
GENERIC_CLOCKEVENTS_MIN_ADJUST. This ensures that any failures are
always retried with an increasing minimum adjustment step.
That, however, suffers from a different issue as described and discussed
here: https://patchwork.kernel.org/patch/8909491/

Various events can delay these operations and even with notrace they
might still fail, but as it stands now, even if the code using them does
a retry, the latency I've observed with tracing enabled is often too
long to ever succeed.

Marcin