From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e31.co.us.ibm.com (e31.co.us.ibm.com [32.97.110.149]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "e31.co.us.ibm.com", Issuer "Equifax" (verified OK)) by ozlabs.org (Postfix) with ESMTP id E188067B6B for ; Wed, 30 Aug 2006 19:43:40 +1000 (EST) Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e31.co.us.ibm.com (8.13.8/8.12.11) with ESMTP id k7U9hb5Q010764 for ; Wed, 30 Aug 2006 05:43:37 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.13.6/8.13.6/NCO v8.1.1) with ESMTP id k7U9hbJD261936 for ; Wed, 30 Aug 2006 03:43:37 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id k7U9hb2S022125 for ; Wed, 30 Aug 2006 03:43:37 -0600 From: Arnd Bergmann To: linuxppc-dev@ozlabs.org Subject: Re: [PATCH 02/13] IB/ehca: includes Date: Wed, 30 Aug 2006 11:43:34 +0200 References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Message-Id: <200608301143.35320.arnd.bergmann@de.ibm.com> Cc: linux-kernel@vger.kernel.org, openib-general@openib.org, Christoph Raisch , Hoang-Nam Nguyen , Marcus Eder , abergman@de.ibm.com List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Wednesday 30 August 2006 11:13, Hoang-Nam Nguyen wrote: > Further comments/suggestions are appreciated! There are a few places in the driver where you declare external variables (mostly ehca_module and ehca_debug_level) from C files instead of a header. This sometimes leads to bugs when a type changes and is therefore considered bad style. ehca_debug_level is already declared in a header so you should not need any other declaration. For ehca_module, the usage pattern is very uncommon. Declaring the structure in a header helps a bit, but I don't really see the need for this structure at all. Each member of the struct seems to be used mostly in a single file, so I would declare it statically in there. E.g. in drivers/infiniband/hw/ehca/ehca_pd.c, you can do static struct kmem_cache *ehca_pd_cache; int ehca_init_pd_cache(void) { ehca_pd_cache = kmem_cache_init("ehca_cache_pd", sizeof(struct ehca_pd), 0, SLAB_HWCACHE_ALIGN, NULL, NULL); if (!ehca_pd_cache) return -ENOMEM; return 0; } void ehca_cleanup_pd_cache(void) { if (ehca_pd_cache) kmem_cache_destroy(ehca_pd_cache); } Moreover, for some of your more heavily used caches, you may want to look into using constructor/destructor calls to speed up allocation. Arnd <>< From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750764AbWH3Jnj (ORCPT ); Wed, 30 Aug 2006 05:43:39 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750785AbWH3Jnj (ORCPT ); Wed, 30 Aug 2006 05:43:39 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:35774 "EHLO e32.co.us.ibm.com") by vger.kernel.org with ESMTP id S1750764AbWH3Jni (ORCPT ); Wed, 30 Aug 2006 05:43:38 -0400 From: Arnd Bergmann Organization: IBM Deutschland Entwicklung GmbH To: linuxppc-dev@ozlabs.org Subject: Re: [PATCH 02/13] IB/ehca: includes Date: Wed, 30 Aug 2006 11:43:34 +0200 User-Agent: KMail/1.9.1 Cc: Hoang-Nam Nguyen , abergman@de.ibm.com, linux-kernel@vger.kernel.org, openib-general@openib.org, Christoph Raisch , Marcus Eder References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200608301143.35320.arnd.bergmann@de.ibm.com> Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday 30 August 2006 11:13, Hoang-Nam Nguyen wrote: > Further comments/suggestions are appreciated! There are a few places in the driver where you declare external variables (mostly ehca_module and ehca_debug_level) from C files instead of a header. This sometimes leads to bugs when a type changes and is therefore considered bad style. ehca_debug_level is already declared in a header so you should not need any other declaration. For ehca_module, the usage pattern is very uncommon. Declaring the structure in a header helps a bit, but I don't really see the need for this structure at all. Each member of the struct seems to be used mostly in a single file, so I would declare it statically in there. E.g. in drivers/infiniband/hw/ehca/ehca_pd.c, you can do static struct kmem_cache *ehca_pd_cache; int ehca_init_pd_cache(void) { ehca_pd_cache = kmem_cache_init("ehca_cache_pd", sizeof(struct ehca_pd), 0, SLAB_HWCACHE_ALIGN, NULL, NULL); if (!ehca_pd_cache) return -ENOMEM; return 0; } void ehca_cleanup_pd_cache(void) { if (ehca_pd_cache) kmem_cache_destroy(ehca_pd_cache); } Moreover, for some of your more heavily used caches, you may want to look into using constructor/destructor calls to speed up allocation. Arnd <><