From patchwork Tue Nov 11 14:37:11 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andrew Jones X-Patchwork-Id: 40576 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-lb0-f198.google.com (mail-lb0-f198.google.com [209.85.217.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 588EC218DE for ; Tue, 11 Nov 2014 14:38:52 +0000 (UTC) Received: by mail-lb0-f198.google.com with SMTP id 10sf5452060lbg.1 for ; Tue, 11 Nov 2014 06:38:51 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:message-id:references :mime-version:in-reply-to:user-agent:cc:subject:precedence:list-id :list-unsubscribe:list-archive:list-post:list-help:list-subscribe :errors-to:sender:x-original-sender :x-original-authentication-results:mailing-list:content-type :content-disposition; bh=i17wGRQV4JjJLJBvspn2bdupQJ0yieIhFMgMieUVKEU=; b=HIVzigu6hUqdBIP4+vNBZwQnYmhHtGtjZWOXV4HyF0MNwUWbGMq20LxPad0wVOiMVC 81nqHO+uJY7oM00KdOmvwOqOp+AmztdYUKZLtQm3vkcIUexBLEoRPUNWelsphHg54e/d H+bS94a820ixvjiHly9siShZ7BDjteefW87ECKrsGSgA2xMTcVq3RfYLTW413J9WLNuv dFFoJGoNJXRRWB9OeOviIAAzhrWQsV+cx57Lrhw9u7sZjhCOqHZ3dRnQLIlyscOEdiCC QGnQNTUiDvR6zUvW128WO0QDkvtqX/O+ZdHwntLk96GMtba2H1qJAbeqCbHckJnp7Ice BOVg== X-Gm-Message-State: ALoCoQlzOn/EdnMv8+q/wrCX4FwaoBVy5wFhdYoBM+LwwUhV3JmMvAHY8dH3+tarod6VxSxDgWpn X-Received: by 10.181.11.193 with SMTP id ek1mr5538328wid.0.1415716731298; Tue, 11 Nov 2014 06:38:51 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.203.167 with SMTP id kr7ls557583lac.94.gmail; Tue, 11 Nov 2014 06:38:50 -0800 (PST) X-Received: by 10.112.42.198 with SMTP id q6mr15776554lbl.69.1415716730351; Tue, 11 Nov 2014 06:38:50 -0800 (PST) Received: from mail-la0-f44.google.com (mail-la0-f44.google.com. [209.85.215.44]) by mx.google.com with ESMTPS id d3si31773367lbc.14.2014.11.11.06.38.50 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 11 Nov 2014 06:38:50 -0800 (PST) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.44 as permitted sender) client-ip=209.85.215.44; Received: by mail-la0-f44.google.com with SMTP id gf13so9748414lab.3 for ; Tue, 11 Nov 2014 06:38:50 -0800 (PST) X-Received: by 10.112.52.37 with SMTP id q5mr36183483lbo.32.1415716730252; Tue, 11 Nov 2014 06:38:50 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.184.201 with SMTP id ew9csp265044lbc; Tue, 11 Nov 2014 06:38:49 -0800 (PST) X-Received: by 10.194.80.100 with SMTP id q4mr54622223wjx.15.1415716729374; Tue, 11 Nov 2014 06:38:49 -0800 (PST) Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id fu1si35633468wjb.120.2014.11.11.06.38.49 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 11 Nov 2014 06:38:49 -0800 (PST) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Received: from localhost ([::1]:49109 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoCal-0000tA-Ev for patch@linaro.org; Tue, 11 Nov 2014 09:38:47 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58355) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoCZP-0007gO-N2 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 09:37:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XoCZJ-0004qm-Iq for qemu-devel@nongnu.org; Tue, 11 Nov 2014 09:37:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51834) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoCZJ-0004nv-C9 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 09:37:17 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sABEbF4U022796 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Tue, 11 Nov 2014 09:37:15 -0500 Received: from hawk.usersys.redhat.com (dhcp-1-153.brq.redhat.com [10.34.1.153]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sABEbBnj019559 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=NO); Tue, 11 Nov 2014 09:37:14 -0500 Date: Tue, 11 Nov 2014 15:37:11 +0100 From: Andrew Jones To: Eduardo Habkost Message-ID: <20141111143709.GA14394@hawk.usersys.redhat.com> References: <1415376280-14130-1-git-send-email-drjones@redhat.com> <1415376280-14130-3-git-send-email-drjones@redhat.com> <20141111124100.GA4985@thinpad.lan.raisama.net> MIME-Version: 1.0 In-Reply-To: <20141111124100.GA4985@thinpad.lan.raisama.net> User-Agent: Mutt/1.5.23 (2014-03-12) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: pbonzini@redhat.com, qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH 2/3] vl: sanity check cpu topology X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: , List-Help: , List-Subscribe: , Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: qemu-devel-bounces+patch=linaro.org@nongnu.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: drjones@redhat.com X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.44 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 Content-Disposition: inline On Tue, Nov 11, 2014 at 10:41:00AM -0200, Eduardo Habkost wrote: > On Fri, Nov 07, 2014 at 05:04:39PM +0100, Andrew Jones wrote: > > smp_parse allows partial or complete cpu topology to be given. > > In either case there may be inconsistencies in the input which > > are currently not sounding any alarms. In some cases the input > > is even being silently corrected. We shouldn't do this. Add > > warnings when input isn't adding up right, and even abort when > > the complete cpu topology has been input, but isn't correct. > > > > Signed-off-by: Andrew Jones > > So, we are fixing bugs and changing behavior on two different cases > here: > > 1) when all options are provided and they aren't enough for smp_cpus; > 2) when one option was missing, but the existing calculation was > incorrect because of division truncation. 3) when threads were provided, but incorrect, we silently changed it. I thought you wanted to fix this one right now too. > > I don't think we need to keep compatibility on (1) because the user is > obviously providing an invalid configuration. That's why I suggested we > implemented it in 2.2. And it is safer because we won't be silently > changing behavior: QEMU is going to abort and the mistake will be easily > detected. > > But (2) is fixing a QEMU bug, not user error. The user may be unaware of > the bug, and will get a silent ABI change once upgrading to a newer > QEMU. We can keep it rounding down, unless the result is zero, as the current code does. How about keeping the new warning? Nah, let's drop it. Who actually cares about warnings anyway... > > I suggest fixing only (1) by now and keeping the behavior for (2) on > QEMU 2.2. Something like: > > if (sockets == 0) { > /* keep existing code for sockets == 0 */ > } else if (cores == 0) { > /* keep existing code for cores == 0 */ > } else if (threads == 0) { > /* keep existing code for threads == 0 */ This doesn't exist with current code. Adding an 'if (threads == 0)' case is fix (3). > } else { > /* new code: */ > if (sockets * cores * threads < cpus) { > fprintf(stderr, "cpu topology: error: " > "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n", > sockets, cores, threads, cpus); > exit(1); > } > } > > Below is a v2 I can post if it looks good to you. From: Andrew Jones Date: Fri, 7 Nov 2014 15:45:07 +0100 Subject: [PATCH v2] vl: sanity check cpu topology smp_parse allows partial or complete cpu topology to be given. In either case there may be inconsistencies in the input which are currently not sounding any alarms. In some cases the input is even being silently corrected. Stop silently adjusting input and abort when the complete cpu topology has been input, but isn't correct. Signed-off-by: Andrew Jones --- vl.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/vl.c b/vl.c index 9d9855092ab4a..e686fd21e266f 100644 --- a/vl.c +++ b/vl.c @@ -1288,16 +1288,39 @@ static void smp_parse(QemuOpts *opts) if (cores == 0) { threads = threads > 0 ? threads : 1; cores = cpus / (sockets * threads); - } else { - threads = cpus / (cores * sockets); + if (cpus % (sockets * threads)) { + /* The calculation resulted in a fractional number, so we + * need to adjust it. The below adjustment is wrong, it + * should be '+= 1', but we need to keep it this way for + * compatibility. + */ + cores = cores > 0 ? cores : 1; + } + } else if (threads == 0) { + threads = cpus / (sockets * cores); + if (cpus % (sockets * cores)) { + /* The calculation resulted in a fractional number, so we + * need to adjust it. The below adjustment is wrong, it + * should be '+= 1', but we need to keep it this way for + * compatibility. + */ + threads = threads > 0 ? threads : 1; + } } } + if (sockets * cores * threads < cpus) { + fprintf(stderr, "cpu topology: error: " + "sockets (%u) * cores (%u) * threads (%u) < smp_cpus (%u)\n", + sockets, cores, threads, cpus); + exit(1); + } + max_cpus = qemu_opt_get_number(opts, "maxcpus", 0); smp_cpus = cpus; - smp_cores = cores > 0 ? cores : 1; - smp_threads = threads > 0 ? threads : 1; + smp_cores = cores; + smp_threads = threads; }