[v5,11/14] software node: move small properties inline when copying
diff mbox series

Message ID 20191011230721.206646-12-dmitry.torokhov@gmail.com
State Superseded, archived
Headers show
Series
  • software node: add support for reference properties
Related show

Commit Message

Dmitry Torokhov Oct. 11, 2019, 11:07 p.m. UTC
When copying/duplicating set of properties, move smaller properties that
were stored separately directly inside property entry structures. We can
move:

- up to 8 bytes from U8 arrays
- up to 4 words
- up to 2 double words
- one U64 value
- one or 2 strings.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/base/swnode.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Andy Shevchenko Oct. 15, 2019, 12:20 p.m. UTC | #1
On Fri, Oct 11, 2019 at 04:07:18PM -0700, Dmitry Torokhov wrote:
> When copying/duplicating set of properties, move smaller properties that
> were stored separately directly inside property entry structures. We can
> move:
> 
> - up to 8 bytes from U8 arrays
> - up to 4 words
> - up to 2 double words
> - one U64 value
> - one or 2 strings.

Can you show where you extract such values?

> +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> +		/* We have an opportunity to move the data inline */
> +		const void *tmp = dst->pointer;
> +

> +		memcpy(&dst->value, tmp, dst->length);

...because this is strange trick.

> +		dst->is_inline = true;
> +
> +		kfree(tmp);
> +	}
Dmitry Torokhov Oct. 15, 2019, 6:25 p.m. UTC | #2
On Tue, Oct 15, 2019 at 03:20:28PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 11, 2019 at 04:07:18PM -0700, Dmitry Torokhov wrote:
> > When copying/duplicating set of properties, move smaller properties that
> > were stored separately directly inside property entry structures. We can
> > move:
> > 
> > - up to 8 bytes from U8 arrays
> > - up to 4 words
> > - up to 2 double words
> > - one U64 value
> > - one or 2 strings.
> 
> Can you show where you extract such values?

the "value" union's largest member is u64, which is 8 bytes. Strings are
pointers, so on 32-bit arches you can stuff 2 pointers into 8 bytes,
while on 64-bits you have space for only one.

> 
> > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > +		/* We have an opportunity to move the data inline */
> > +		const void *tmp = dst->pointer;
> > +
> 
> > +		memcpy(&dst->value, tmp, dst->length);
> 
> ...because this is strange trick.

Not sure what is so strange about it. You just take data that is stored
separately and move it into the structure, provided that it is not too
big (i.e. it does not exceed sizeof(value union) size).

> 
> > +		dst->is_inline = true;
> > +
> > +		kfree(tmp);
> > +	}
> 

Thanks.
Andy Shevchenko Oct. 16, 2019, 7:48 a.m. UTC | #3
On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> On Tue, Oct 15, 2019 at 03:20:28PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 11, 2019 at 04:07:18PM -0700, Dmitry Torokhov wrote:
> > > When copying/duplicating set of properties, move smaller properties that
> > > were stored separately directly inside property entry structures. We can
> > > move:
> > > 
> > > - up to 8 bytes from U8 arrays
> > > - up to 4 words
> > > - up to 2 double words
> > > - one U64 value
> > > - one or 2 strings.
> > 
> > Can you show where you extract such values?
> 
> the "value" union's largest member is u64, which is 8 bytes. Strings are
> pointers, so on 32-bit arches you can stuff 2 pointers into 8 bytes,
> while on 64-bits you have space for only one.
> 
> > 
> > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > +		/* We have an opportunity to move the data inline */
> > > +		const void *tmp = dst->pointer;
> > > +
> > 
> > > +		memcpy(&dst->value, tmp, dst->length);
> > 
> > ...because this is strange trick.
> 
> Not sure what is so strange about it. You just take data that is stored
> separately and move it into the structure, provided that it is not too
> big (i.e. it does not exceed sizeof(value union) size).

You store a value as union, but going to read as a member of union?
I'm pretty sure it breaks standard rules.
Dmitry Torokhov Oct. 16, 2019, 4:01 p.m. UTC | #4
On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> > On Tue, Oct 15, 2019 at 03:20:28PM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 11, 2019 at 04:07:18PM -0700, Dmitry Torokhov wrote:
> > > > When copying/duplicating set of properties, move smaller properties that
> > > > were stored separately directly inside property entry structures. We can
> > > > move:
> > > > 
> > > > - up to 8 bytes from U8 arrays
> > > > - up to 4 words
> > > > - up to 2 double words
> > > > - one U64 value
> > > > - one or 2 strings.
> > > 
> > > Can you show where you extract such values?
> > 
> > the "value" union's largest member is u64, which is 8 bytes. Strings are
> > pointers, so on 32-bit arches you can stuff 2 pointers into 8 bytes,
> > while on 64-bits you have space for only one.
> > 
> > > 
> > > > +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> > > > +		/* We have an opportunity to move the data inline */
> > > > +		const void *tmp = dst->pointer;
> > > > +
> > > 
> > > > +		memcpy(&dst->value, tmp, dst->length);
> > > 
> > > ...because this is strange trick.
> > 
> > Not sure what is so strange about it. You just take data that is stored
> > separately and move it into the structure, provided that it is not too
> > big (i.e. it does not exceed sizeof(value union) size).
> 
> You store a value as union, but going to read as a member of union?
> I'm pretty sure it breaks standard rules.

No, I move the values _in place_ of the union, and the data is always
fetched via void pointers. And copying data via char * or memcpy() is
allowed even in C99 and C11.

But I am wondering why are we actually worrying about all of this? The
kernel is gnu89 and I think is going to stay this way because we use
initializers with a cast in a lot of places:

#define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
        (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)

and C99 and gnu99 do not allow this. See
https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/

Thanks.
Andy Shevchenko Oct. 16, 2019, 4:18 p.m. UTC | #5
On Wed, Oct 16, 2019 at 09:01:26AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:

> > You store a value as union, but going to read as a member of union?
> > I'm pretty sure it breaks standard rules.
> 
> No, I move the values _in place_ of the union, and the data is always
> fetched via void pointers. And copying data via char * or memcpy() is
> allowed even in C99 and C11.
> 
> But I am wondering why are we actually worrying about all of this? The
> kernel is gnu89 and I think is going to stay this way because we use
> initializers with a cast in a lot of places:
> 
> #define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
>         (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
> 
> and C99 and gnu99 do not allow this. See
> https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/

This is simple not a cast.
Andy Shevchenko Oct. 16, 2019, 4:23 p.m. UTC | #6
On Wed, Oct 16, 2019 at 07:18:45PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 09:01:26AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> 
> > > You store a value as union, but going to read as a member of union?
> > > I'm pretty sure it breaks standard rules.
> > 
> > No, I move the values _in place_ of the union, and the data is always
> > fetched via void pointers. And copying data via char * or memcpy() is
> > allowed even in C99 and C11.
> > 
> > But I am wondering why are we actually worrying about all of this? The
> > kernel is gnu89 and I think is going to stay this way because we use
> > initializers with a cast in a lot of places:
> > 
> > #define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
> >         (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
> > 
> > and C99 and gnu99 do not allow this. See
> > https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/
> 
> This is simple not a cast.

4.62 Compound literals in C99
ISO C99 supports compound literals. A compound literal looks like a cast
followed by an initializer. Its value is an object of the type specified in the
cast, containing the elements specified in the initializer. It is an lvalue.
Dmitry Torokhov Oct. 16, 2019, 4:44 p.m. UTC | #7
On Wed, Oct 16, 2019 at 07:23:08PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 16, 2019 at 07:18:45PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 16, 2019 at 09:01:26AM -0700, Dmitry Torokhov wrote:
> > > On Wed, Oct 16, 2019 at 10:48:57AM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 15, 2019 at 11:25:53AM -0700, Dmitry Torokhov wrote:
> > 
> > > > You store a value as union, but going to read as a member of union?
> > > > I'm pretty sure it breaks standard rules.
> > > 
> > > No, I move the values _in place_ of the union, and the data is always
> > > fetched via void pointers. And copying data via char * or memcpy() is
> > > allowed even in C99 and C11.
> > > 
> > > But I am wondering why are we actually worrying about all of this? The
> > > kernel is gnu89 and I think is going to stay this way because we use
> > > initializers with a cast in a lot of places:
> > > 
> > > #define __RAW_SPIN_LOCK_UNLOCKED(lockname)      \
> > >         (raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
> > > 
> > > and C99 and gnu99 do not allow this. See
> > > https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/
> > 
> > This is simple not a cast.
> 
> 4.62 Compound literals in C99
> ISO C99 supports compound literals. A compound literal looks like a cast
> followed by an initializer. Its value is an object of the type specified in the
> cast, containing the elements specified in the initializer. It is an lvalue.

Yes, these are compound literals. And they can not be used as
initializers:

https://lore.kernel.org/lkml/CAHk-=wgXBV57mz46ZB5XivjiSBGkM0cjuvnU2OWyfRF=+41NPQ@mail.gmail.com/

Thanks.
Dmitry Torokhov Oct. 17, 2019, 4:04 p.m. UTC | #8
On Fri, Oct 11, 2019 at 04:07:18PM -0700, Dmitry Torokhov wrote:
> When copying/duplicating set of properties, move smaller properties that
> were stored separately directly inside property entry structures. We can
> move:
> 
> - up to 8 bytes from U8 arrays
> - up to 4 words
> - up to 2 double words
> - one U64 value
> - one or 2 strings.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/base/swnode.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
> index ae4b24ee2a54..546fc1b20095 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -277,6 +277,16 @@ static int property_entry_copy_data(struct property_entry *dst,
>  		dst->value = src->value;
>  	}
>  
> +	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
> +		/* We have an opportunity to move the data inline */
> +		const void *tmp = dst->pointer;
> +
> +		memcpy(&dst->value, tmp, dst->length);
> +		dst->is_inline = true;
> +
> +		kfree(tmp);
> +	}

This chunk needs to be moved to after dst->length is assigned.  I'll
send updated version after I get more feedback.

> +
>  	dst->length = src->length;
>  	dst->type = src->type;
>  	dst->name = kstrdup(src->name, GFP_KERNEL);

Thanks.

Patch
diff mbox series

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ae4b24ee2a54..546fc1b20095 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -277,6 +277,16 @@  static int property_entry_copy_data(struct property_entry *dst,
 		dst->value = src->value;
 	}
 
+	if (!dst->is_inline && dst->length <= sizeof(dst->value)) {
+		/* We have an opportunity to move the data inline */
+		const void *tmp = dst->pointer;
+
+		memcpy(&dst->value, tmp, dst->length);
+		dst->is_inline = true;
+
+		kfree(tmp);
+	}
+
 	dst->length = src->length;
 	dst->type = src->type;
 	dst->name = kstrdup(src->name, GFP_KERNEL);