Subject: Re: [PATCH] powerpc/tm: Clean up duplication of code

From: Michael Ellerman <[email-masked]>
Date: Wed, 11 May 2016 21:12:08 +1000

On Wed, 2016-05-11 at 17:44 +1000, Rashmica wrote:
> On 11/05/16 15:03, Balbir Singh wrote:
> > On 11/05/16 14:55, Rashmica Gupta wrote:
> > > The same logic for tm_abort appears twice, so pull it out into a
> > > function.
> > > 
> > > diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> > > index 7635b1c6b5da..1cef8f5aee9b 100644
> > > --- a/arch/powerpc/mm/hash_utils_64.c
> > > +++ b/arch/powerpc/mm/hash_utils_64.c
> > > @@ -1318,6 +1318,25 @@ out_exit:
> > >   	local_irq_restore(flags);
> > >   }
> > >   
> > > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > > +	/* Transactions are not aborted by tlbiel, only tlbie.

Can you fix up the comment formatting while you're there, should be:

/*
 * Transactions are not aborted by tlbiel, only tlbie.

> > > +	 * Without, syncing a page back to a block device w/ PIO could pick up
> > > +	 * transactional data (bad!) so we force an abort here.  Before the
> > > +	 * sync the page will be made read-only, which will flush_hash_page.
> > > +	 * BIG ISSUE here: if the kernel uses a page from userspace without
> > > +	 * unmapping it first, it may see the speculated version.
> > > +	 */
> > > +static inline void abort_tm(int local)
> > > +{
> > > +	if (local && cpu_has_feature(CPU_FTR_TM) &&
> > > +	    current->thread.regs &&
> > > +	    MSR_TM_ACTIVE(current->thread.regs->msr)) {
> > > +		tm_enable();
> > > +		tm_abort(TM_CAUSE_TLBI);
> > > +	}
> > > +}
> > While your at this do
> > 
> > #else
> > 
> > static inline void abort_tm(int local)
> > {
> > }
> If I'm doing that, wouldn't it make more sense to write:
> 
> +static inline void abort_tm(int local)

It needs a better name. Perhaps tlb_flush_abort_tm() ?

> +{
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +	if (local && cpu_has_feature(CPU_FTR_TM) &&
> +	    current->thread.regs &&
> +	    MSR_TM_ACTIVE(current->thread.regs->msr)) {
> +		tm_enable();
> +		tm_abort(TM_CAUSE_TLBI);
> +	}
> +#endif
> +}

In this case it would make some sense.

But the goal is "no #ifdef's in C code", which that would violate.

And if you have two functions that need to be inside the #ifdef then the above
doesn't work anymore.

So the preferred style is:

  #ifdef CONFIG_FOO
  static void foo(int bar)
  {
  	...
  }
  #else /* !CONFIG_FOO */
  static inline foo(int bar) { }
  #endif

And when it expands you end up with:

  #ifdef CONFIG_FOO
  static void foo(int bar)
  {
  	...
  }

  static void foo_new(int bar)
  {
  	...
  }
  #else /* !CONFIG_FOO */
  static inline foo(int bar) { }
  static inline foo_new(int bar) { }
  #endif

cheers