From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH v3 2/3] libxc/xentrace: Replace xc_tbuf_set_cpu_mask with CPU mask with xc_cpumap_t instead of uint32_t Date: Mon, 30 Mar 2015 14:04:50 -0400 Message-ID: <20150330180450.GA21779@l.oracle.com> References: <1427211559-15185-1-git-send-email-konrad.wilk@oracle.com> <1427211559-15185-3-git-send-email-konrad.wilk@oracle.com> <5519755D.9000103@eu.citrix.com> <20150330165411.GC13141@l.oracle.com> <551988E3.4050600@eu.citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Yce3X-0006vd-W3 for xen-devel@lists.xenproject.org; Mon, 30 Mar 2015 18:05:00 +0000 Content-Disposition: inline In-Reply-To: <551988E3.4050600@eu.citrix.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: George Dunlap Cc: xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On Mon, Mar 30, 2015 at 06:33:23PM +0100, George Dunlap wrote: > On 03/30/2015 05:54 PM, Konrad Rzeszutek Wilk wrote: > > On Mon, Mar 30, 2015 at 05:10:05PM +0100, George Dunlap wrote: > >> On 03/24/2015 03:39 PM, Konrad Rzeszutek Wilk wrote: > >>> We replace the implementation of xc_tbuf_set_cpu_mask with > >>> an xc_cpumap_t instead of a uint32_t. This means we can use an > >>> arbitrary bitmap without being limited to the 32-bits as > >>> previously we were. Furthermore since there is only one > >>> user of xc_tbuf_set_cpu_mask we just replace it and > >>> its user in one go. > >>> > >>> We also add an macro which can be used by both libxc and > >>> xentrace. > >>> > >>> And update the man page to describe this behavior. > >>> > >>> Signed-off-by: Konrad Rzeszutek Wilk > >>> Acked-by: Ian Campbell > >>> [libxc pieces] > >> > >> OK, so I just took the time to wrap my brain around this patch more, and > >> I'm afraid I think it's almost entirely wrong. :-/ > >> > >> To sum up: > >> > >> 1. There's no reason to pass the number of bits to xc_tbuf_set_cpu_mask. > >> The caller should just pass a fully-filled xc_cpumask_t. > >> > >> 2. xentrace shouldn't rely on open-coded knowledge about the length of > >> xc_cpumask_t; it should call xc_get_cpumask_size() and use that. > >> > >> 3. If the user doesn't pass a mask, then the mask should just be left > >> unchanged; it shouldn't silently go and set all the bits in the cpumask. > > > > Which would be then an cpumask with zero CPUs set? > > No -- what xentrace does at the moment: if there's no cpumask passed in, > just don't call xc_tbuf_set_cpu_mask() at all. Leave it the way it was > when you found it. When Xen boots will start with all cpus set; if a > previous caller has changed it, just leave it the way you found it. Aah, I missed that. > > I think the current behavior is fine, but my opinion isn't very strong. > Feel free to try to make the case that the current UI is wrong and we > *should* set everything again by default. But in that case, 1) it > should be a separate patch, 2) we should follow the same principle for Correct. > the evtmask. > > >> + map[i] = (mask >> (i*8)) & 0xff; > > > > I was never sure of the right syntax for this so in my original patch I > > had (mask >> (i * 8)) && 0xff; > > Hmm? I just looked at the last two patches and they had '&'. && is > logical and; it will give you either 0 or 1. Sorry, not '&&'. It was the '(i * 8)' vs '(i*8)' > > > So do you want to take this patch and put it into your series (making > all the changes you suggest), or do you want me to polish it up and send > it separately? I will take it in, do the changes, and also test it on a large machine. Thought should I wait until you are done looking at the other patch? > > -George