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

[Dmitry Torokhov]

On Sat, Sep 07, 2019 at 09:03:48PM +0300, Andy Shevchenko wrote:
> On Sat, Sep 07, 2019 at 10:37:24AM -0700, Dmitry Torokhov wrote:
> > 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.
> 
> We can put it one level up to be a sibling to value / pointer unions.
> In that case is_array can be anything (we just don't care).

I think it would be better if you sketched out your proposed data
structure(s) so we are talking about the same things. But please note
that when you are dealing with property arrays we need to keep the easy
way of defining them, which means we should not be splitting individual
entries.

> 
> Actually strings aren't inlined.

Right. Maybe I should clean it up as well.

> 
> > > By the way, don't we need to update property_entry_{get,set}_pointer()?
> > 
> > I do not see these, where are they?
> 
> swnode.c. I meant property_{get,set}_pointer().

Yes, I think you are right about property_set_pointer() at least.

> 
> > > > > > +	.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.
> 
> Thus, moving to upper layer makes more sense. Right?
> 
> > > > 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.
> 
> Nope, since strings are not inlined AFAICS.

But u64 values are.

Thanks.

-- 
Dmitry