From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755034AbdDMP2A (ORCPT ); Thu, 13 Apr 2017 11:28:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41556 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754542AbdDMP14 (ORCPT ); Thu, 13 Apr 2017 11:27:56 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com E37E84E040 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=riel@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com E37E84E040 Message-ID: <1492097269.2896.8.camel@redhat.com> Subject: Re: [RFC 3/3] sched/topology: Different sched groups must not have the same balance cpu From: Rik van Riel To: Lauro Ramos Venancio , linux-kernel@vger.kernel.org Cc: lwang@redhat.com, Mike Galbraith , Peter Zijlstra , Thomas Gleixner , Ingo Molnar Date: Thu, 13 Apr 2017 11:27:49 -0400 In-Reply-To: <1492091769-19879-4-git-send-email-lvenanci@redhat.com> References: <1492091769-19879-1-git-send-email-lvenanci@redhat.com> <1492091769-19879-4-git-send-email-lvenanci@redhat.com> Organization: Red Hat, Inc. Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 13 Apr 2017 15:27:51 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-04-13 at 10:56 -0300, Lauro Ramos Venancio wrote: > Currently, the group balance cpu is the groups's first CPU. But with > overlapping groups, two different groups can have the same first CPU. > > This patch uses the group mask to mark all the CPUs that have a > particular group as its main sched group. The group balance cpu is > the > first group CPU that is also in the mask. > This is not your fault, but this code is really hard to understand. Your comments tell me what the code does, but not really why.  > +++ b/kernel/sched/topology.c > @@ -477,27 +477,31 @@ enum s_alloc { >  }; >   >  /* > - * Build an iteration mask that can exclude certain CPUs from the > upwards > - * domain traversal. > + * An overlap sched group may not be present in all CPUs that > compose the > + * group. So build the mask, marking all the group CPUs where it is > present. >   * >   * Asymmetric node setups can result in situations where the domain > tree is of >   * unequal depth, make sure to skip domains that already cover the > entire >   * range. > - * > - * In that case build_sched_domains() will have terminated the > iteration early > - * and our sibling sd spans will be empty. Domains should always > include the > - * CPU they're built on, so check that. >   */ Why are we doing this? Could the comment explain why things need to be this way? > - for_each_cpu(i, span) { > + for_each_cpu(i, sg_span) { >   sibling = *per_cpu_ptr(sdd->sd, i); > - if (!cpumask_test_cpu(i, > sched_domain_span(sibling))) > + > + /* > +  * Asymmetric node setups: skip domains that are > already > +  * done. > +  */ > + if (!sibling->groups) > + continue; > + What does this mean? I would really like it if this code was a little better documented.  This is not something I can put on you for most of the code, but I can at least ask it for the code you are adding :) The same goes for the rest of this patch.