From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755317AbbK3XXH (ORCPT ); Mon, 30 Nov 2015 18:23:07 -0500 Received: from mga11.intel.com ([192.55.52.93]:49799 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755249AbbK3XXG (ORCPT ); Mon, 30 Nov 2015 18:23:06 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,366,1444719600"; d="scan'208";a="863685167" From: Alexander Shishkin To: Mathieu Poirier , gregkh@linuxfoundation.org Cc: zhang.chunyan@linaro.org, mike.leach@arm.com, tor@ti.com, al.grant@arm.com, pawel.moll@arm.com, fainelli@broadcom.com, linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, Mathieu Poirier Subject: Re: [PATCH V5 21/26] coresight: etm-perf: new PMU driver for ETM tracers In-Reply-To: <1448849687-5724-22-git-send-email-mathieu.poirier@linaro.org> References: <1448849687-5724-1-git-send-email-mathieu.poirier@linaro.org> <1448849687-5724-22-git-send-email-mathieu.poirier@linaro.org> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 01 Dec 2015 01:23:01 +0200 Message-ID: <87poyrysve.fsf@ashishki-desk.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Mathieu Poirier writes: > +static void etm_event_destroy(struct perf_event *event) {} > + > +static int etm_event_init(struct perf_event *event) > +{ > + if (event->attr.type != etm_pmu.type) > + return -ENOENT; > + > + if (event->cpu >= nr_cpu_ids) > + return -EINVAL; > + > + event->destroy = etm_event_destroy; You don't have to do this if it's a nop, event::destroy can be NULL. > + > + return 0; > +} > +static void *alloc_event_data(int cpu) > +{ > + int lcpu, size; > + cpumask_t *mask; > + struct etm_cpu_data *cpu_data; > + struct etm_event_data *event_data; > + > + /* First get memory for the session's data */ > + event_data = kzalloc(sizeof(struct etm_event_data), GFP_KERNEL); > + if (!event_data) Looks like a whitespace mixup. > + return NULL; > + > + /* Make sure nothing disappears under us */ > + get_online_cpus(); > + size = num_online_cpus(); > + > + mask = &event_data->mask; > + if (cpu != -1) > + cpumask_set_cpu(cpu, mask); > + else > + cpumask_copy(mask, cpu_online_mask); It would be nice to have a comment somewhere here explaining that you have to set up tracer on each cpu in case of per-thread counter and why. We must have discussed this, but I forgot already. Btw, do you want to also set 'size' to 1 for cpu != -1 case? > + put_online_cpus(); > + > + /* Allocate an array of cpu_data to work with */ > + event_data->cpu_data = kcalloc(size, > + sizeof(struct etm_cpu_data *), > + GFP_KERNEL); > + if (!event_data->cpu_data) > + goto free_event_data; > + > + /* Allocate a cpu_data for each CPU this event is dealing with */ > + for_each_cpu(lcpu, mask) { > + cpu_data = kzalloc(sizeof(struct etm_cpu_data), GFP_KERNEL); > + if (!cpu_data) > + goto free_event_data; > + > + event_data->cpu_data[lcpu] = cpu_data; > + } Wouldn't it be easier to allocate the whole thing with one event_data->cpu_data = kcalloc(size, sizeof(struct etm_cpu_data), GFP_KERNEL); ? Regards, -- Alex From mboxrd@z Thu Jan 1 00:00:00 1970 From: alexander.shishkin@linux.intel.com (Alexander Shishkin) Date: Tue, 01 Dec 2015 01:23:01 +0200 Subject: [PATCH V5 21/26] coresight: etm-perf: new PMU driver for ETM tracers In-Reply-To: <1448849687-5724-22-git-send-email-mathieu.poirier@linaro.org> References: <1448849687-5724-1-git-send-email-mathieu.poirier@linaro.org> <1448849687-5724-22-git-send-email-mathieu.poirier@linaro.org> Message-ID: <87poyrysve.fsf@ashishki-desk.ger.corp.intel.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Mathieu Poirier writes: > +static void etm_event_destroy(struct perf_event *event) {} > + > +static int etm_event_init(struct perf_event *event) > +{ > + if (event->attr.type != etm_pmu.type) > + return -ENOENT; > + > + if (event->cpu >= nr_cpu_ids) > + return -EINVAL; > + > + event->destroy = etm_event_destroy; You don't have to do this if it's a nop, event::destroy can be NULL. > + > + return 0; > +} > +static void *alloc_event_data(int cpu) > +{ > + int lcpu, size; > + cpumask_t *mask; > + struct etm_cpu_data *cpu_data; > + struct etm_event_data *event_data; > + > + /* First get memory for the session's data */ > + event_data = kzalloc(sizeof(struct etm_event_data), GFP_KERNEL); > + if (!event_data) Looks like a whitespace mixup. > + return NULL; > + > + /* Make sure nothing disappears under us */ > + get_online_cpus(); > + size = num_online_cpus(); > + > + mask = &event_data->mask; > + if (cpu != -1) > + cpumask_set_cpu(cpu, mask); > + else > + cpumask_copy(mask, cpu_online_mask); It would be nice to have a comment somewhere here explaining that you have to set up tracer on each cpu in case of per-thread counter and why. We must have discussed this, but I forgot already. Btw, do you want to also set 'size' to 1 for cpu != -1 case? > + put_online_cpus(); > + > + /* Allocate an array of cpu_data to work with */ > + event_data->cpu_data = kcalloc(size, > + sizeof(struct etm_cpu_data *), > + GFP_KERNEL); > + if (!event_data->cpu_data) > + goto free_event_data; > + > + /* Allocate a cpu_data for each CPU this event is dealing with */ > + for_each_cpu(lcpu, mask) { > + cpu_data = kzalloc(sizeof(struct etm_cpu_data), GFP_KERNEL); > + if (!cpu_data) > + goto free_event_data; > + > + event_data->cpu_data[lcpu] = cpu_data; > + } Wouldn't it be easier to allocate the whole thing with one event_data->cpu_data = kcalloc(size, sizeof(struct etm_cpu_data), GFP_KERNEL); ? Regards, -- Alex