Re: [RFC][PATCH] flexible array implementation

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Tue, 2009-07-21 at 12:47 -0400, Mike Waychison wrote:
> > +/*
> > + * This is meant to replace cases where an array-like
> > + * structure has gotten to big to fit into kmalloc()
> 
> s/to/too/g

Gah, thanks.

> > + * and the developer is getting tempted to use
> > + * vmalloc().
> > + */
> > +
> > +struct flex_array {
> > +	union {
> > +		struct {
> > +			int nr_elements;
> > +			int element_size;
> > +			struct flex_array_part *parts[0];
> > +		};
> > +		/*
> > +		 * This little trick makes sure that
> > +		 * sizeof(flex_array) == PAGE_SIZE
> > +		 */
> > +		char padding[FLEX_ARRAY_BASE_SIZE];
> > +	};
> > +};
> > +
> > +struct flex_array *alloc_flex_array(int element_size, int total, gfp_t flags);
> > +void free_flex_array(struct flex_array *fa);
> > +int flex_array_put(struct flex_array *fa, void *src, gfp_t flags);
> > +void *flex_array_get(struct flex_array *fa, int element_nr);
> 
> Would a single routine that acted like an lvalue be a bit clearer?  Here 
> it looks like the interface is a push to write, but random access to 
> read.  Something like:
> 
> void **__flex_array_idx(struct flex_rray *fa, int element_nr);
> #define flex_array_idx(fa, element_nr) (*__flex_array_idx(fa, element_nr))
> 
> may be a bit easier to read so users of the interface could just do:
> 
> flex_array_idx(fa, n) = pointer;
> pointer = flex_array_idx(fa, n);
> 
> I guess this would require that flex_array only store actual pointers 
> though, instead of objects.  Hmm.

Yeah, possibly.  

I was hoping I'd get some suggestions for places in the kernel that
could use this.  I'll see if another interface looks better once I get
some users. :)

> Is there any reason you defer allocation of parts to flex_array_put() 
> given that alloc_flex_array() is told the total number of elements?

Yeah, I guess it's just in the hope that it won't all get filled.  I
think I also started with it not being passed the total size.  But, I
figured later on that it was better to fail as early as possible rather
than screwing the user later on when we fill up. 

> > +struct flex_array_part {
> > +	char elements[FLEX_ARRAY_PART_SIZE];
> > +};
> > +
> > +static inline int __elements_per_part(int element_size)
> > +{
> > +	return FLEX_ARRAY_PART_SIZE / element_size;
> 
> Micro-optimization, but perhaps the general case could use a bit shift 
> stored inside the flex_array?  That way we could avoid excessive divide 
> operations.

Possibly, but that of course precludes having objects with
non-power-of-two sizes being packed very well.  It's certainly worth
thinking about.

> > +void *flex_array_get(struct flex_array *fa, int element_nr)
> > +{
> > +	int part_nr = fa_element_to_part_nr(fa, element_nr);
> > +	struct flex_array_part *part;
> > +	int offset;
> > +
> > +	if (part_nr > __nr_part_ptrs())
> 
> if (part_nr >= __nr_part_ptrs())

Yep.  Good catch.

> > +		return NULL;
> 
> ERR_PTR(-EINVAL) ?

I guess this helps users differentiate bad arguments from memory
allocation failures.  But, I'd rather not make users check ERR_PTR()
just because I want to keep it simple.  

> Would it make sense to WARN here?  A more precise test could be to 
> compare element_nr vs fa->nr_elements?

My immediate goal here was just to avoid oopses and overflows.  You're
right that fa->nr_elements would be more precise.  But, I guess if you
overflow an array you get an oops, so why should this be any different?
A WARN() is potentially noisy, but way less noisy than an oops.  

Thanks for the feedback!

-- Dave