Re: exofs/ore: allocation of _ore_get_io_state()

[Boaz Harrosh <bharrosh@panasas.com>]

On 04/16/2012 05:53 PM, Idan Kedar wrote:

> _ore_get_io_state is supposed to allocate a struct ore_io_state, which is
> variable length. Setting aside the uglification argument which gave birth
> to the patch "pnfs-obj: Uglify objio_segment allocation for the sake of
> the principle :-(", the above function checks if the memory it needs to
> allocate is bigger than a page, and if so, separates it into two
> allocations. why is this done?
> 
> from include/linux/slab.h:
>> /*
>> * The largest kmalloc size supported by the slab allocators is
>> * 32 megabyte (2^25) or the maximum allocatable page order if that is
>> * less than 32 MB.
>> *
>> * WARNING: Its not easy to increase this value since the allocators have
>> * to do various tricks to work around compiler limitations in order to
>> * ensure proper constant folding.
>> */
>> #define KMALLOC_SHIFT_HIGH      ((MAX_ORDER + PAGE_SHIFT - 1) <= 25 ? \
>>                                (MAX_ORDER + PAGE_SHIFT - 1) : 25)
>>
>> #define KMALLOC_MAX_SIZE        (1UL << KMALLOC_SHIFT_HIGH)
>> #define KMALLOC_MAX_ORDER       (KMALLOC_SHIFT_HIGH - PAGE_SHIFT)
> 
> If kmalloc can allocate 32MB, why check one page, like so?
> 
> from fs/exofs/ore.c:
>> struct __alloc_all_io_state {
>> 	struct ore_io_state ios;
>> 	struct ore_per_dev_state per_dev[numdevs];
>> 	union {
>> 		struct osd_sg_entry sglist[sgs_per_dev * numdevs];
>> 		struct page *pages[num_par_pages];
>> 	};
>> } *_aios;
>>
>> if (likely(sizeof(*_aios) <= PAGE_SIZE)) {
>> 	_aios = kzalloc(sizeof(*_aios), GFP_KERNEL);
>> 	if (unlikely(!_aios)) {
>> 		EXOFS_DBGMSG("Failed kzalloc bytes=%zd\n",
>> 			   sizeof(*_aios));
>> 		*pios = NULL;
>> 		return -ENOMEM;
>> 	}
>> 	pages = num_par_pages ? _aios->pages : NULL;
>> 	sgilist = sgs_per_dev ? _aios->sglist : NULL;
>> 	ios = &_aios->ios;
>> } else {
>> 	struct __alloc_small_io_state {
>> 		struct ore_io_state ios;
>> 		struct ore_per_dev_state per_dev[numdevs];
>> 	} *_aio_small;
>> 	union __extra_part {
>> 		struct osd_sg_entry sglist[sgs_per_dev * numdevs];
>> 		struct page *pages[num_par_pages];
>> 	} *extra_part;
>> 		_aio_small = kzalloc(sizeof(*_aio_small), GFP_KERNEL);
>> 	if (unlikely(!_aio_small)) {
>> 		EXOFS_DBGMSG("Failed alloc first part bytes=%zd\n",
>> 			   sizeof(*_aio_small));
>> 		*pios = NULL;
>> 		return -ENOMEM;
>> 	}
>> 	extra_part = kzalloc(sizeof(*extra_part), GFP_KERNEL);
>> 	if (unlikely(!extra_part)) {
>> 		EXOFS_DBGMSG("Failed alloc second part bytes=%zd\n",
>> 			   sizeof(*extra_part));
>> 		kfree(_aio_small);
>> 		*pios = NULL;
>> 		return -ENOMEM;
>> 	}
>> 		pages = num_par_pages ? extra_part->pages : NULL;
>> 	sgilist = sgs_per_dev ? extra_part->sglist : NULL;
>> 	/* In this case the per_dev[0].sgilist holds the pointer to
>> 	 * be freed
>> 	 */
>> 	ios = &_aio_small->ios;
>> 	ios->extra_part_alloc = true;
>> }
> 
> Is there any point to check if the memory is greater than 32MB?
> 
> 


In theory it can allocate 32MB, in slab. I'm not sure about slob and slub.

But in practice contiguous physical pages allocation tends to fail very
fast on a system that was up a couple of hours. So we avoid it as plage.

Past testing with tables bigger than PAGE_SIZE on the IO path gave
catastrophic results. (Again once the system is up for a while and
had a chance to fragment physical address space)

The all Kernel point of the use of sg-lists is so not to allocate
contiguous physical pages and to not have to use virtual-memory.

This is done all over the Kernel. MAX_BIO_SIZE max-sg-table ...

(BTW I saw this mail by chance. If you direct it to me I see it
 for sure)

Cheers
Boaz