Re: [PATCH v2 1/3] software node: implement reference properties

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Sat, Sep 07, 2019 at 08:12:51PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 07, 2019 at 09:32:40AM -0700, Dmitry Torokhov wrote:
> > On Sat, Sep 07, 2019 at 07:08:19PM +0300, Andy Shevchenko wrote:
> > > On Fri, Sep 06, 2019 at 03:26:09PM -0700, Dmitry Torokhov wrote:
> 
> > > > +	} else if (src->type == DEV_PROP_REF) {
> > > > +		/* All reference properties must be arrays */
> > > > +		return -EINVAL;
> > > 
> > > Hmm... What about to duplicate pointer under value union and use is_array to
> > > distinguish which one to use? Because...
> > 
> > Then we have to special-case copying this entry, similar to the pains we
> > are going with the strings.
> 
> I can't see it as a pain. Simple do the same kmemdup() for the case when
> is_array = false and DEV_TYPE_REF?

And then you need to make sure it is freed on error paths and when we
remove property entries. This requires more checks and code. In contrast
we already know how to handle out of line objects of arbitrary size.

The only reason we have inline strings is because for shorter strings we
save 4/8 bytes.

> 
> By the way, don't we need to update property_entry_{get,set}_pointer()?

I do not see these, where are they?

> 
> > > > +	.is_array = true,						\
> > > 
> > > I really don't like this "cheating".
> > 
> > This is not cheating. Any single value can be represented as an array of
> > one element. Actually, the only reason we have this "is_array" business
> > is because for scalar values and short strings it is much cheaper to
> > store single value in-line instead of out of line + pointer, especially
> > on 64 bit arches.
> 
> Yes, and this is a lot of benefit!

Yes, nobody argues against it. Here however we are dealing with a larger
structure. There is absolutely no benefit of trying to separate single
value vs array here.

> 
> > If you want we can change is_array into is_inline.
> 
> Nope, is_array is exactly what it tells us about the content. Its functional
> load is to distinguish which union (value vs. pointer) we are using.

No, it signifies whether the value is stored within property entry or
outside. I can fit probably 8 bytes arrays into property entry
structure, in which case is_array will definitely not reflect the data
type.

It is the type-specific accessors that know how to parse and fetch data
from properties.

Thanks.

-- 
Dmitry