Date:   Wed, 11 Mar 2020 12:14:18 -0700
From:   Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>
To:     Reinette Chatre <reinette.chatre@...el.com>, shuah@...nel.org,
        linux-kselftest@...r.kernel.org
Cc:     tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        tony.luck@...el.com, babu.moger@....com, james.morse@....com,
        ravi.v.shankar@...el.com, fenghua.yu@...el.com, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 10/13] selftests/resctrl: Change Cache Allocation
 Technology (CAT) test

Hi Reinette,

On Wed, 2020-03-11 at 10:03 -0700, Reinette Chatre wrote:
> Hi Sai,
> 
> On 3/10/2020 6:59 PM, Sai Praneeth Prakhya wrote:
> > On Tue, 2020-03-10 at 15:14 -0700, Reinette Chatre wrote:
> > > Hi Sai,
> > > 
> > > Not just specific to this patch but I think the prevalent use of global
> > > variables that are initialized/used or allocated/released from a variety
> > > of places within the code is creating traps. I seemed to have stumbled
> > > on a few during this review so far but it is hard to keep track of and I
> > > am not confident that I caught them all. Having the code be symmetrical
> > > (allocate and free from same area or initialize and use from same area)
> > > does help to avoid such complexity.
> > 
> > Sure! makes sense. I will try to wrap them up in some meaningful
> > structures to
> > pass around functions and will see if everything still works as expected.
> > If
> > not, I will comment why a particular variable needs to be global.
> > 
> > > This patch and the patch that follows are both quite large and difficult
> > > to keep track of all the collected changes. There seems to be
> > > opportunity for separating it into logical changes. Some of my comments
> > > may be just because I could not keep track of all that is changed at the
> > > same time.
> > 
> > Ok.. makes sense. The main reason this patch and the next patch are large 
> > because they do two things
> > 1. Remove previous CAT/CQM test case
> > 2. Add new CAT/CQM test cases
> > 
> > Since the new test cases are not just logical extensions or fixing some
> > bugs
> > in previous test cases, the patch might not be readable. I am thinking to
> > split this at-least like this
> > 1. A patch to remove CAT test case
> > 2. A patch to remove CQM test case
> > 3. Patches that just add CAT and CQM (without other changes)
> > 
> > Please let me know if you think otherwise
> 
> I think this patch can be split up into logical changes without breaking
> the tests along the way. In my original review I identified two changes
> that can be split out. Other things that can be split out:
> - have CAT test take shareable bits into account
> - enable measurement of cache references (addition of this new perf
> event attribute, hooks to get measurements, etc.)
> - transition CAT test to use "perf rate" measurement instead of "perf count"
> - etc.

I think we could split the patch like this but I am unable to see the benefit
of doing so.. (Sorry! if I misunderstood what you meant).

As CAT and CQM test cases are buggy (CAT is not testing CAT at all) and we are
not attempting to fix them by incremental changes but completely changing the
test plan itself (i.e. the way the test works), so why not just remove older
test cases and add new test? I thought this might be more easier for review
i.e. to see the new test case all at once. Don't you think so?

> 
> > > On 3/6/2020 7:40 PM, Sai Praneeth Prakhya wrote:
> 
> [SNIP]
> 
> > > > -static struct perf_event_attr pea_llc_miss;
> > > > +static struct perf_event_attr pea_llc_miss, pea_llc_access;
> > > >  static struct read_format rf_cqm;
> > > > -static int fd_lm;
> > > > +static int fd_lm, fd_la;
> > > >  char llc_occup_path[1024];
> > > >  
> > > >  static void initialize_perf_event_attr(void)
> > > > @@ -27,15 +27,30 @@ static void initialize_perf_event_attr(void)
> > > >  	pea_llc_miss.inherit = 1;
> > > >  	pea_llc_miss.exclude_guest = 1;
> > > >  	pea_llc_miss.disabled = 1;
> > > > +
> > > > +	pea_llc_access.type = PERF_TYPE_HARDWARE;
> > > > +	pea_llc_access.size = sizeof(struct perf_event_attr);
> > > > +	pea_llc_access.read_format = PERF_FORMAT_GROUP;
> > > > +	pea_llc_access.exclude_kernel = 1;
> > > > +	pea_llc_access.exclude_hv = 1;
> > > > +	pea_llc_access.exclude_idle = 1;
> > > > +	pea_llc_access.exclude_callchain_kernel = 1;
> > > > +	pea_llc_access.inherit = 1;
> > > > +	pea_llc_access.exclude_guest = 1;
> > > > +	pea_llc_access.disabled = 1;
> > > > +
> > > 
> > > This initialization appears to duplicate the initialization done above.
> > > Perhaps this function could be a wrapper that calls an initialization
> > > function with pointer to perf_event_attr that initializes structure the
> > > same?
> > 
> > I did think about a wrapper but since pea_llc_access and pea_llc_miss are
> > global variables, I thought passing them as variables might not look good
> > (why
> > do we want to pass a global variable?). I will try and see if I can make
> > these
> > local variables.
> 
> My goal was to avoid the duplicated code to initialize them identically.

I agree that duplicate should always be avoided.

> It is not clear to me why you think that would not look good.

I didn't mean that avoiding duplicate code doesn't look good.. what I meant
was passing global variables around will not look good.

> Perhaps I have not thought it through correctly ...

No.. I think the right thing to do here is not use global variable and hence
avoid duplicate code.

Regards,
Sai

Powered by blists - more mailing lists