From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752052AbdJZApW convert rfc822-to-8bit (ORCPT ); Wed, 25 Oct 2017 20:45:22 -0400 Received: from mail.efficios.com ([167.114.142.141]:53149 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751911AbdJZApV (ORCPT ); Wed, 25 Oct 2017 20:45:21 -0400 Date: Thu, 26 Oct 2017 00:47:45 +0000 (UTC) From: Mathieu Desnoyers To: Stephen Bates Cc: Daniel Mentz , logang@deltatee.com, Jonathan Corbet , Andrew Morton , Will Deacon , linux-kernel Message-ID: <1655113933.49689.1508978865628.JavaMail.zimbra@efficios.com> In-Reply-To: <893BAB3D-6B3C-46BB-A16E-E64BF278D90E@raithlin.com> References: <1508945574-25984-1-git-send-email-sbates@raithlin.com> <893BAB3D-6B3C-46BB-A16E-E64BF278D90E@raithlin.com> Subject: Re: [PATCH] genalloc: Make the avail variable an atomic64_t MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT X-Originating-IP: [167.114.142.141] X-Mailer: Zimbra 8.7.11_GA_1854 (ZimbraWebClient - FF52 (Linux)/8.7.11_GA_1854) Thread-Topic: genalloc: Make the avail variable an atomic64_t Thread-Index: AQHTTaaMVAi2tIvJJ0ery9xrTz9EPKL02X0AgABKFgBvJPeAiw== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- On Oct 26, 2017, at 12:20 AM, Stephen Bates sbates@raithlin.com wrote: >> I found that genalloc is very slow for large chunk sizes because >> bitmap_find_next_zero_area has to grind through that entire bitmap. >> Hence, I recommend avoiding genalloc for large chunk sizes. > > Thanks for the feedback Daniel! We have been doing 16GiB without any noticeable > issues. > >> I'm thinking how this would behave on a 32 bit ARM platform > > I don’t think people would be doing such big allocations on 32 bit (ARM > systems). It would not make sense for them to be doing >4GB anyway. > >>> --- a/lib/genalloc.c >>> +++ b/lib/genalloc.c >>> @@ -194,7 +194,7 @@ int gen_pool_add_virt(struct gen_pool *pool, unsigned long >>> virt, phys_addr_t phy >>> chunk->phys_addr = phys; >>> chunk->start_addr = virt; >>> chunk->end_addr = virt + size - 1; >>> - atomic_set(&chunk->avail, size); >>> + atomic64_set(&chunk->avail, size); > >> Isn't size defined as a size_t type which is 32 bit wide on ARM? How >> can you ever set chunk->avail to anything larger than 2^32 - 1? > > I did consider changing this type but it seems like there would never be a need > to set this value to more than 4GiB on 32 bit systems. > >>> @@ -464,7 +464,7 @@ size_t gen_pool_avail(struct gen_pool *pool) >>> >>> rcu_read_lock(); >>> list_for_each_entry_rcu(chunk, &pool->chunks, next_chunk) >>> - avail += atomic_read(&chunk->avail); >>> + avail += atomic64_read(&chunk->avail); >> >>avail is defined as size_t (32 bit). Aren't you going to overflow that variable? > > Again, I don’t think people on 32 bit systems will be doing >4GB assignments so > it would not be an issue. We have atomic_long_t for that. Please use it instead. It will be 64-bit on 64-bit archs, and 32-bit on 32-bit archs, which seems to fit your purpose here. Thanks, Mathieu > > Stephen -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com