| Message ID | 20191011230721.206646-12-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | software node: add support for reference properties | expand |
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); > + }
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.
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.
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.
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.
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.
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.
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.
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);
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(+)