From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755258AbbIALPp (ORCPT ); Tue, 1 Sep 2015 07:15:45 -0400 Received: from casper.infradead.org ([85.118.1.10]:45550 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753276AbbIALPn (ORCPT ); Tue, 1 Sep 2015 07:15:43 -0400 Date: Tue, 1 Sep 2015 13:15:33 +0200 From: Peter Zijlstra To: Taku Izumi Cc: linux-kernel@vger.kernel.org, mingo@redhat.com, acme@kernel.org, hpa@zytor.com, x86@kernel.org, jolsa@redhat.com Subject: Re: [PATCH v2][RESEND] perf, x86: Fix multi-segment problem of perf_event_intel_uncore Message-ID: <20150901111533.GQ19282@twins.programming.kicks-ass.net> References: <1440672872-129663-1-git-send-email-izumi.taku@jp.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1440672872-129663-1-git-send-email-izumi.taku@jp.fujitsu.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 27, 2015 at 07:54:32PM +0900, Taku Izumi wrote: > This patch fixes ths problem by introducing segment-aware pci2phy_map instead. > > v1 -> v2: > - Extract method named uncore_pcibus_to_physid to avoid repetetion of > retrieving phys_id code So close and yet so far... > + raw_spin_lock(&pci2phy_map_lock); > + list_for_each_entry(map, &pci2phy_map_head, list) { > + if (map->segment == segment) { > + found = true; > + break; > + } > + } > + if (!found) { > + map = kmalloc(sizeof(struct pci2phy_map), GFP_KERNEL); > + if (map) { > + map->segment = segment; > + map->pbus_to_physid[bus] = 0; > + list_add_tail(&map->list, &pci2phy_map_head); > + } > + } else { > + map->pbus_to_physid[bus] = 0; > + } > + raw_spin_unlock(&pci2phy_map_lock); > + > + segment = pci_domain_nr(ubox_dev->bus); > + raw_spin_lock(&pci2phy_map_lock); > + list_for_each_entry(map, &pci2phy_map_head, list) { > + if (map->segment == segment) { > + found = true; > break; > } > } > + if (!found) { > + map = kmalloc(sizeof(struct pci2phy_map), GFP_KERNEL); > + if (map) { > + map->segment = segment; > + list_add_tail(&map->list, &pci2phy_map_head); > + } > + } > + if (map) { > + /* > + * every three bits in the Node ID mapping register > + * maps to a particular node. > + */ > + for (i = 0; i < 8; i++) { > + if (nodeid == ((config >> (3 * i)) & 0x7)) { > + map->pbus_to_physid[bus] = i; > + break; > + } > + } > + > + } > + raw_spin_unlock(&pci2phy_map_lock); > } Nothing there strikes you as repetitive ? Also, no mention on the -ENOMEM handling, and you simply _CANNOT_ do a GFP_KERNEL alloc while holding a spinlock, so I bet you didn't actually test the patch either. Maybe something like the below? Equally untested. --- --- a/arch/x86/kernel/cpu/perf_event_intel_uncore.c +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore.c @@ -23,8 +23,8 @@ struct event_constraint uncore_constrain int uncore_pcibus_to_physid(struct pci_bus *bus) { - int phys_id = -1; struct pci2phy_map *map; + int phys_id = -1; raw_spin_lock(&pci2phy_map_lock); list_for_each_entry(map, &pci2phy_map_head, list) { @@ -38,6 +38,26 @@ int uncore_pcibus_to_physid(struct pci_b return phys_id; } +struct __find_pci2phy(int segment) +{ + struct pci2phy_map *map; + + lockdep_assert_held(&pci2phy_map_lock); + + list_for_each_entry(map, &pci2phy_map_head, list) { + if (map->segment == segment) + return map; + } + + map = kmalloc(sizeof(struct pci2phy_map), GFP_ATOMIC); + if (map) { + map->segment = segment; + list_add_tail(&map->list, &pci2phy_map_head); + } + + return map; +} + ssize_t uncore_event_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) { --- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snb.c @@ -433,22 +433,12 @@ static int snb_pci2phy_map_init(int devi segment = pci_domain_nr(dev->bus); raw_spin_lock(&pci2phy_map_lock); - list_for_each_entry(map, &pci2phy_map_head, list) { - if (map->segment == segment) { - found = true; - break; - } - } - if (!found) { - map = kmalloc(sizeof(struct pci2phy_map), GFP_KERNEL); - if (map) { - map->segment = segment; - map->pbus_to_physid[bus] = 0; - list_add_tail(&map->list, &pci2phy_map_head); - } - } else { - map->pbus_to_physid[bus] = 0; + map = __find_phy2pci(segment); + if (!map) { + raw_spin_unlock(&pci2phy_map_lock); + return -ENOMEM; } + map->pbus_to_physid[bus] = 0; raw_spin_unlock(&pci2phy_map_lock); pci_dev_put(dev); --- a/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c +++ b/arch/x86/kernel/cpu/perf_event_intel_uncore_snbep.c @@ -1112,31 +1112,21 @@ static int snbep_pci2phy_map_init(int de segment = pci_domain_nr(ubox_dev->bus); raw_spin_lock(&pci2phy_map_lock); - list_for_each_entry(map, &pci2phy_map_head, list) { - if (map->segment == segment) { - found = true; - break; - } - } - if (!found) { - map = kmalloc(sizeof(struct pci2phy_map), GFP_KERNEL); - if (map) { - map->segment = segment; - list_add_tail(&map->list, &pci2phy_map_head); - } + map = __find_pci2phy(segment); + if (!map) { + err = -ENOMEM; + break; } - if (map) { - /* - * every three bits in the Node ID mapping register - * maps to a particular node. - */ - for (i = 0; i < 8; i++) { - if (nodeid == ((config >> (3 * i)) & 0x7)) { - map->pbus_to_physid[bus] = i; - break; - } - } + /* + * every three bits in the Node ID mapping register + * maps to a particular node. + */ + for (i = 0; i < 8; i++) { + if (nodeid == ((config >> (3 * i)) & 0x7)) { + map->pbus_to_physid[bus] = i; + break; + } } raw_spin_unlock(&pci2phy_map_lock); }