From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chao Peng Subject: Re: [PATCH v7 01/14] x86: add socket_to_cpumask Date: Wed, 20 May 2015 11:13:58 +0800 Message-ID: <20150520031358.GH8239@pengc-linux.bj.intel.com> References: <1431075415-21917-2-git-send-email-chao.p.peng@linux.intel.com> <555A0384020000780007B2A7@mail.emea.novell.com> <20150519061224.GA8239@pengc-linux.bj.intel.com> <555AF441020000780007B6A1@mail.emea.novell.com> <20150519064721.GB8239@pengc-linux.bj.intel.com> <555AF9B4020000780007B6E5@mail.emea.novell.com> <20150519071039.GC8239@pengc-linux.bj.intel.com> <555B0309020000780007B722@mail.emea.novell.com> <20150519095120.GG8239@pengc-linux.bj.intel.com> <555B2A0C020000780007B8B0@mail.emea.novell.com> Reply-To: Chao Peng Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <555B2A0C020000780007B8B0@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, will.auld@intel.com, keir@xen.org, dgdegra@tycho.nsa.gov List-Id: xen-devel@lists.xenproject.org On Tue, May 19, 2015 at 11:18:20AM +0100, Jan Beulich wrote: > >>> On 19.05.15 at 11:51, wrote: > > On Tue, May 19, 2015 at 08:31:53AM +0100, Jan Beulich wrote: > >> >>> On 19.05.15 at 09:10, wrote: > >> > On Tue, May 19, 2015 at 07:52:04AM +0100, Jan Beulich wrote: > >> >> >>> On 19.05.15 at 08:47, wrote: > >> >> > On Tue, May 19, 2015 at 07:28:49AM +0100, Jan Beulich wrote: > >> >> >> >>> On 19.05.15 at 08:12, wrote: > >> >> >> > On Mon, May 18, 2015 at 02:21:40PM +0100, Jan Beulich wrote: > >> >> >> >> >>> On 08.05.15 at 10:56, wrote: > >> >> >> >> > @@ -112,6 +115,8 @@ static int __devinit MP_processor_info_x(struct > >> >> > mpc_config_processor *m, > >> >> >> >> > { > >> >> >> >> > int ver, apicid, cpu = 0; > >> >> >> >> > > >> >> >> >> > + total_cpus++; > >> >> >> >> > + > >> >> >> >> > if (!(m->mpc_cpuflag & CPU_ENABLED)) { > >> >> >> >> > if (!hotplug) > >> >> >> >> > ++disabled_cpus; > >> >> >> >> > >> >> >> >> Is there a reason you can't use disabled_cpus and avoid adding yet > >> >> >> >> another variable? > >> >> >> > > >> >> >> > The problem is not with disabled_cpus but with num_processors, which > >> >> >> > does not keep the original detected cpus in current code. > >> >> >> > Hence 'total_cpus = disabled_cpus + num_processors' may not be correct > >> >> >> > in some cases. > >> >> >> > >> >> >> Please be more specific about when this is a problem (I do note that > >> >> >> I'm aware that the equation will not always hold, but during my > >> >> >> inspection while reviewing your change I didn't see that this would > >> >> >> ever become problematic). > >> >> > > >> >> > What I really need is the original cpu count enumerated from MADT. If > >> >> > not introduce total_cpus then the only way getting it AFAICS is > >> >> > 'disabled_cpus + num_processors'. > >> >> > > >> >> > The problem is that MP_processor_info_x() have some earlier returns > >> >> > before increasing num_processors. In those cases, the cpu detected will > >> >> > neither counted to disabled_cpus nor num_processors, which means > >> >> > 'disabled_cpus + num_processors' is potentially small than what I need. > >> >> > >> >> As said - I understand this. But you still fail to explain under what > >> >> (realistic, i.e. other than someone bogusly setting NR_CPUS=1) > >> >> conditions this ends up being a problem. > >> > > >> > As we calculate nr_sockets with: > >> > > >> > nr_sockets = total_cpus / _cpus_per_socket__ > >> > > >> > If the calculated total_cpus is smaller than the actual cpu count on the > >> > hardware, then the nr_sockets is also potentially smaller than the > >> > actual socket count on the hardware. This is not the expectation. > >> > >> Sure - but you still don't say what is going to go wrong. Remember, > >> when I asked you to change to the total count I gave an explicit > >> example: Use of "nosmp" would have yielded a zero nr_sockets in > >> the earlier code. Yet with the sum of num_processors and > >> disabled_cpus this can't happen anymore afaict. > > > > "nosmp" only has side effect on max_cpus and nr_cpu_ids, but they are > > never used at all when calculating nr_sockets. So I can't see any reason > > why with "num_processors + disabled_cpus" the nr_sockets would not be > > zero, I think this is a bug that I should fix in nosmp case. > > Did you really mean to say "would _not_ be zero"? Because afaict > the above resolves to "would ever be zero". Yes, I mean "would ever be zero". > > >> Hence I'm looking > >> forward to you detailing the conditions under which you would see > >> an issue without introducing total_cpus. > > > > As said before, with "num_processors + disabled_cpus" I may get a > > smaller nr_sockets than the machine actually has. This is my exact > > problem: I may miss enumerating some CAT-enabled sockets. While the > > assumption is that I will follow your suggestion to make nr_socket >= > > the socket count that the machine actually has. > > Please can you stop repeating yourself? Yes, nr_sockets can get > underestimated this way, but the cases where total_cpus != > num_processors + disabled_cpus are - afaict - such that this in > the end does no harm. Other than the original case of nr_sockets > ending up being zero when "nosmp". So to convince me of the > opposite I'm afraid you won't get around constructing an explicit > example where things break (and hence my earlier hint regarding > NR_CPUS=1, as I wouldn't count this as a valid example). My original thought is that with newly introduced total_cpus, nr_sockets is less likely to be underestimated than with "num_processors + disabled_cpus", if not now but for the future. But perhaps I understood it incorrectly. While this is really not a big deal for me. And "num_processors + disabled_cpus" does work for me now so I feel comfortable to use it. I do have a problem when coming up with the final codes, there are two possible ways: 1) Make both num_processors and disabled_cpus public so that they can be used when calculating nr_sockets. 2) Add another public function set_nr_sockets in mpparse.c (just like set_nr_cpu_ids does). Which do you prefer? Thanks, Chao