linux-kernel - Re: [PATCH v3 4/7] perf stat: Use affinity for closing file descriptors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191104233553.zcjw2u64pggixkka@two.firstfloor.org>
Date:   Mon, 4 Nov 2019 15:35:53 -0800
From:   Andi Kleen <andi@...stfloor.org>
To:     Jiri Olsa <jolsa@...hat.com>
Cc:     Andi Kleen <andi@...stfloor.org>, acme@...nel.org,
        jolsa@...nel.org, eranian@...gle.com, linux-kernel@...r.kernel.org,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH v3 4/7] perf stat: Use affinity for closing file
 descriptors

> >  
> > -	evlist__for_each_entry_reverse(evlist, evsel)
> > -		evsel__close(evsel);
> > +	if (affinity__setup(&affinity) < 0)
> > +		return;
> > +	cpus = evlist__cpu_iter_start(evlist);
> > +	cpumap__for_each_cpu (cpus, i, cpu) {
> > +		affinity__set(&affinity, cpu);
> 
> whats the point of affinity->changed flags when we call
> affinity__set unconditionaly? I think we can do without
> it, becase we'll always endup calling affinity__set

No we don't in the per thread case (without -a) 

In this case affinity is never set because there is no cpu.

I added it just to make the strace look nicer in this case.

> 
> however, it seems superfluous to always allocate those
> bitmaps, while we need just the current cpus that we
> run on and also that is probably questionable
> 
> could we put 'struct affinity' to 'struct evlist'
> and get rid of all affinity__setup/cleanup calls?
> (apart from those in evlist__init and evlist__delete)

affinity setup/cleanup is essentially push/pop for the affinity
state. For setup while it could be in theory moved
it would be a bad API because if someone sets up a evlist
inside an affinity region it would save the wrong state.

For cleanup there's nothing that would call it to reset
the affinity.

They could be made global, but that's somewhat ugly
and might also break with threading.


-Andi