From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752642AbdGMSfq (ORCPT ); Thu, 13 Jul 2017 14:35:46 -0400 Received: from mga06.intel.com ([134.134.136.31]:56625 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751337AbdGMSfp (ORCPT ); Thu, 13 Jul 2017 14:35:45 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,355,1496127600"; d="scan'208";a="1151265029" Date: Thu, 13 Jul 2017 11:37:22 -0700 (PDT) From: Shivappa Vikas X-X-Sender: vikas@vshiva-Udesk To: Thomas Gleixner cc: Shivappa Vikas , Vikas Shivappa , x86@kernel.org, linux-kernel@vger.kernel.org, hpa@zytor.com, peterz@infradead.org, "Shankar, Ravi V" , tony.luck@intel.com, "Yu, Fenghua" Subject: Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support In-Reply-To: Message-ID: References: <1498503368-20173-1-git-send-email-vikas.shivappa@linux.intel.com> <1498503368-20173-14-git-send-email-vikas.shivappa@linux.intel.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 6 Jul 2017, Thomas Gleixner wrote: > On Thu, 6 Jul 2017, Shivappa Vikas wrote: >> On Sun, 2 Jul 2017, Thomas Gleixner wrote: >>>> + /* Check whether cpus belong to parent ctrl group */ >>>> + cpumask_andnot(tmpmask, newmask, &pr->cpu_mask); >>>> + if (cpumask_weight(tmpmask)) { >>>> + ret = -EINVAL; >>>> + goto out; >>>> + } >>>> + >>>> + /* Check whether cpus are dropped from this group */ >>>> + cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask); >>>> + if (cpumask_weight(tmpmask)) { >>>> + /* Give any dropped cpus to parent rdtgroup */ >>>> + cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask); >>> >>> This does not make any sense. The check above verifies that all cpus in >>> newmask belong to the parent->cpu_mask. If they don't then you return >>> -EINVAL, but here you give them back to parent->cpu_mask. How is that >>> supposed to work? You never get into this code path! >> >> The parent->cpu_mask always is the parent->cpus_valid_mask if i understand >> right. With monitor group, the cpu is present is always present in "one" >> ctrl_mon group and one mon_group. And the mon group can have only cpus in its >> parent. May be it needs a comment? (its explaind in the documentation patch). > > Sigh, the code needs to be written in a way that it is halfways obvious > what's going on. > >> # mkdir /sys/fs/resctrl/p1 >> # mkdir /sys/fs/resctrl/p1/mon_groups/m1 >> # echo 5-10 > /sys/fs/resctr/p1/cpus_list >> Say p1 has RMID 2 >> cpus 5-10 have RMID 2 > > So what you say, is that parent is always the resource control group > itself. > > Can we please have a proper distinction in the code? I tripped over that > ambigiousities several times. > > The normal meaning of parent->child relations is that both have the same > type. While this is the case at the implementation detail level (both are > type struct rdtgroup), from a conceptual level they are different: > > parent is a resource group and child is a monitoring group > > That should be expressed in the code, at the very least by variable naming, > so it becomes immediately clear that this operates on two different > entities. > > The proper solution is to have different data types or at least embedd the > monitoring bits in a seperate entity inside of struct rdtgroup. Yes they are conceptually different. There were data which were specific to monitoring only but they share a lot of data. So I was still thinking whats best but kept a type which seperates them both. But the monitoring only data seems like only the 'parent' so we can embed the monitoring bits in a seperate struct (The parent is initialized for ctrl_mon group but never really used). Thanks, Vikas