On 08/02/2018 07:56 PM, Matthew Wilcox wrote:
>
>> struct dma_pool { /* the pool */
>> #define POOL_FULL_IDX 0
>> #define POOL_AVAIL_IDX 1
>> #define POOL_N_LISTS 2
>> struct list_head page_list[POOL_N_LISTS];
>> spinlock_t lock;
>> - size_t size;
>> struct device *dev;
>> - size_t allocation;
>> - size_t boundary;
>> + unsigned int size;
>> + unsigned int allocation;
>> + unsigned int boundary_shift;
>> + unsigned int blks_per_boundary;
>> + unsigned int blks_per_alloc;
> s/size_t/unsigned int/ is a good saving on 64-bit systems. We recently
> did something similar for slab/slub.
I was trying to avoid 64-bit multiply and divide in the fast path. And
the rest of the code uses unsigned int in various places, so big sizes
won't work anyway. But without the conversion from "byte offset" to
"block index" everywhere, the overhead doesn't matter as much. But it
could still be done.
One place where size_t might be appropriate is in show_pools() when
adding together the memory usage of the entire pool (it currently uses
unsigned int). Now that it scales well. ;)
>
>> @@ -141,6 +150,7 @@ static DEVICE_ATTR(pools, 0444, show_pool
>> struct dma_pool *dma_pool_create(const char *name, struct device *dev,
>> size_t size, size_t align, size_t boundary)
>> {
> We should change the API here too.
>
>
There are a lot of kernel API functions that take a size_t, even though
a contiguous chunk of memory >= 4 GiB is pretty rare in the kernel. For
example, dma_alloc_coherent() takes a size_t. I suppose if you take a
size_t you can return an error if the value is bigger than supported.
OTOH, if you take an unsigned int, a huge value could be truncated
before you perform your range check on it. size_t also has the benefit
of implicitly "documenting" the interface because people immediately
know what it means.
