Re: [PATCH RFC 01/19] kallsyms: Simplify update_iter_mod()

[Arnaldo Carvalho de Melo]

Em Thu, May 10, 2018 at 05:02:18PM +0000, Hunter, Adrian escreveu:
> > -----Original Message-----
> > From: Jiri Olsa [mailto:jolsa@xxxxxxxxxx]
> > Sent: Thursday, May 10, 2018 4:02 PM
> > To: Hunter, Adrian <adrian.hunter@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>; Arnaldo Carvalho de Melo
> > <acme@xxxxxxxxxx>; Ingo Molnar <mingo@xxxxxxxxxx>; Peter Zijlstra
> > <peterz@xxxxxxxxxxxxx>; Andy Lutomirski <luto@xxxxxxxxxx>; H. Peter
> > Anvin <hpa@xxxxxxxxx>; Andi Kleen <ak@xxxxxxxxxxxxxxx>; Alexander
> > Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>; Dave Hansen
> > <dave.hansen@xxxxxxxxxxxxxxx>; Joerg Roedel <joro@xxxxxxxxxx>; linux-
> > kernel@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx
> > Subject: Re: [PATCH RFC 01/19] kallsyms: Simplify update_iter_mod()
> > 
> > On Wed, May 09, 2018 at 02:43:30PM +0300, Adrian Hunter wrote:
> > > Simplify logic in update_iter_mod().
> > >
> > > Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > > ---
> > >  kernel/kallsyms.c | 20 ++++++--------------
> > >  1 file changed, 6 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index
> > > a23e21ada81b..eda4b0222dab 100644
> > > --- a/kernel/kallsyms.c
> > > +++ b/kernel/kallsyms.c
> > > @@ -510,23 +510,15 @@ static int update_iter_mod(struct kallsym_iter
> > > *iter, loff_t pos)  {
> > >  	iter->pos = pos;
> > >
> > > -	if (iter->pos_ftrace_mod_end > 0 &&
> > > -	    iter->pos_ftrace_mod_end < iter->pos)
> > > -		return get_ksymbol_bpf(iter);
> > > -
> > > -	if (iter->pos_mod_end > 0 &&
> > > -	    iter->pos_mod_end < iter->pos) {
> > > -		if (!get_ksymbol_ftrace_mod(iter))
> > > -			return get_ksymbol_bpf(iter);
> > > +	if ((!iter->pos_mod_end || iter->pos_mod_end > pos) &&
> > > +	    get_ksymbol_mod(iter))
> > >  		return 1;
> > 
> > hum, should that be iter-> pos_mod_end >= pos ?
> 
> But module_get_kallsym() returned -1 when pos_mod_end was set to pos.

After looking at the reset_iter(), etc, i.e. the lifetime of reading
/proc/kallsyms, I _think_ this is ok, but if it works already, why
simplify it? Because it paves the way for later changes in this function
in this patchset? If so, please state that, and then spelling out why
this is better than before helps with reviewing, please expand on this
cset comment log.

- Arnaldo
 
> > 
> > 
> > > -	}
> > >
> > > -	if (!get_ksymbol_mod(iter)) {
> > > -		if (!get_ksymbol_ftrace_mod(iter))
> > > -			return get_ksymbol_bpf(iter);
> > > -	}
> > > +	if ((!iter->pos_ftrace_mod_end || iter->pos_ftrace_mod_end >
> > pos) &&
> > > +	    get_ksymbol_ftrace_mod(iter))
> > 
> > same here? iter->pos_ftrace_mod_end >= pos
> > 
> > jirka
> > 
> > > +		return 1;
> > >
> > > -	return 1;
> > > +	return get_ksymbol_bpf(iter);
> > >  }
> > >
> > >  /* Returns false if pos at or past end of file. */
> > > --
> > > 1.9.1
> > >