| Message ID | 1475857062-55311-10-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Daniele Di Proietto |
| Headers | show |
CodingStyle.md instructs to group struct members into related groups. Also, changing the relative order of pointers should not make any difference. Could you achieve the same by reordering just the members above the ‘DPIF_UC_ACTION only.’ comment? Jarno > On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> wrote: > > By reordering the data elements in dpif_upcall structure, pad bytes can > be reduced and also a cache line. > > Before: structure size:768, holes:1, sum padbytes:60, cachelines:12 > After: structure size:656, holes:1, sum padbytes:4, cachelines:11 > > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> > --- > lib/dpif.h | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/lib/dpif.h b/lib/dpif.h > index a7c5097..4a4bb3d 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum dpif_upcall_type); > struct dpif_upcall { > /* All types. */ > enum dpif_upcall_type type; > - struct dp_packet packet; /* Packet data. */ > - struct nlattr *key; /* Flow key. */ > - size_t key_len; /* Length of 'key' in bytes. */ > - ovs_u128 ufid; /* Unique flow identifier for 'key'. */ > - struct nlattr *mru; /* Maximum receive unit. */ > - struct nlattr *cutlen; /* Number of bytes shrink from the end. */ > > /* DPIF_UC_ACTION only. */ > - struct nlattr *userdata; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ > - struct nlattr *out_tun_key; /* Output tunnel key. */ > struct nlattr *actions; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ > + struct nlattr *out_tun_key; /* Output tunnel key. */ > + struct nlattr *userdata; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ > + > + struct nlattr *cutlen; /* Number of bytes shrink from the end. */ > + struct nlattr *mru; /* Maximum receive unit. */ > + ovs_u128 ufid; /* Unique flow identifier for 'key'. */ > + struct dp_packet packet; /* Packet data. */ > + struct nlattr *key; /* Flow key. */ > + size_t key_len; /* Length of 'key' in bytes. */ > }; > > /* A callback to notify higher layer of dpif about to be purged, so that > -- > 2.4.11 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
Hello jarno,
Thanks for the feedback, while reordering the members of dpif_upcall, I had to deviate from standards due to below reason.
The dp_packet member has mbuf as first member that starts at new cache line creating hole of size 60 bytes between dpif_upcall_type and dp_packet as pointed below.
struct dpif_upcall {
enum dpif_upcall_type type;
--> 60 bytes hole
/* --- cacheline 1 boundary (64 bytes) --- */
struct dp_packet {
struct rte_mbuf {
/* typedef MARKER */ void * cacheline0[0]; /* 64 0 */
}
struct nlattr * key;
.
.
}
I tried to pack this hole by moving other members in to this space.
Regards,
Bhanu Prakash.
>-----Original Message-----
>From: Jarno Rajahalme [mailto:jarno@ovn.org]
>Sent: Friday, October 7, 2016 10:11 PM
>To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com>
>Cc: dev@openvswitch.org
>Subject: Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall
>structure.
>
>CodingStyle.md instructs to group struct members into related groups. Also,
>changing the relative order of pointers should not make any difference. Could
>you achieve the same by reordering just the members above the
>‘DPIF_UC_ACTION only.’ comment?
>
> Jarno
>
>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy
><bhanuprakash.bodireddy@intel.com> wrote:
>>
>> By reordering the data elements in dpif_upcall structure, pad bytes
>> can be reduced and also a cache line.
>>
>> Before: structure size:768, holes:1, sum padbytes:60, cachelines:12
>> After: structure size:656, holes:1, sum padbytes:4, cachelines:11
>>
>> Signed-off-by: Bhanuprakash Bodireddy
>> <bhanuprakash.bodireddy@intel.com>
>> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
>> ---
>> lib/dpif.h | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index a7c5097..4a4bb3d 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum
>> dpif_upcall_type); struct dpif_upcall {
>> /* All types. */
>> enum dpif_upcall_type type;
>> - struct dp_packet packet; /* Packet data. */
>> - struct nlattr *key; /* Flow key. */
>> - size_t key_len; /* Length of 'key' in bytes. */
>> - ovs_u128 ufid; /* Unique flow identifier for 'key'. */
>> - struct nlattr *mru; /* Maximum receive unit. */
>> - struct nlattr *cutlen; /* Number of bytes shrink from the end. */
>>
>> /* DPIF_UC_ACTION only. */
>> - struct nlattr *userdata; /* Argument to
>OVS_ACTION_ATTR_USERSPACE. */
>> - struct nlattr *out_tun_key; /* Output tunnel key. */
>> struct nlattr *actions; /* Argument to OVS_ACTION_ATTR_USERSPACE.
>*/
>> + struct nlattr *out_tun_key; /* Output tunnel key. */
>> + struct nlattr *userdata; /* Argument to
>OVS_ACTION_ATTR_USERSPACE. */
>> +
>> + struct nlattr *cutlen; /* Number of bytes shrink from the end. */
>> + struct nlattr *mru; /* Maximum receive unit. */
>> + ovs_u128 ufid; /* Unique flow identifier for 'key'. */
>> + struct dp_packet packet; /* Packet data. */
>> + struct nlattr *key; /* Flow key. */
>> + size_t key_len; /* Length of 'key' in bytes. */
>> };
>>
>> /* A callback to notify higher layer of dpif about to be purged, so
>> that
>> --
>> 2.4.11
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
How about making the ‘dp_packet’ member the first member and adding a comment that this should be first? Jarno > On Oct 10, 2016, at 8:42 AM, Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com> wrote: > > Hello jarno, > > Thanks for the feedback, while reordering the members of dpif_upcall, I had to deviate from standards due to below reason. > The dp_packet member has mbuf as first member that starts at new cache line creating hole of size 60 bytes between dpif_upcall_type and dp_packet as pointed below. > > struct dpif_upcall { > enum dpif_upcall_type type; > > --> 60 bytes hole > > /* --- cacheline 1 boundary (64 bytes) --- */ > struct dp_packet { > struct rte_mbuf { > /* typedef MARKER */ void * cacheline0[0]; /* 64 0 */ > > } > struct nlattr * key; > . > . > } > > I tried to pack this hole by moving other members in to this space. > > Regards, > Bhanu Prakash. > >> -----Original Message----- >> From: Jarno Rajahalme [mailto:jarno@ovn.org] >> Sent: Friday, October 7, 2016 10:11 PM >> To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com> >> Cc: dev@openvswitch.org >> Subject: Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall >> structure. >> >> CodingStyle.md instructs to group struct members into related groups. Also, >> changing the relative order of pointers should not make any difference. Could >> you achieve the same by reordering just the members above the >> ‘DPIF_UC_ACTION only.’ comment? >> >> Jarno >> >>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy >> <bhanuprakash.bodireddy@intel.com> wrote: >>> >>> By reordering the data elements in dpif_upcall structure, pad bytes >>> can be reduced and also a cache line. >>> >>> Before: structure size:768, holes:1, sum padbytes:60, cachelines:12 >>> After: structure size:656, holes:1, sum padbytes:4, cachelines:11 >>> >>> Signed-off-by: Bhanuprakash Bodireddy >>> <bhanuprakash.bodireddy@intel.com> >>> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> >>> --- >>> lib/dpif.h | 17 +++++++++-------- >>> 1 file changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/lib/dpif.h b/lib/dpif.h >>> index a7c5097..4a4bb3d 100644 >>> --- a/lib/dpif.h >>> +++ b/lib/dpif.h >>> @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum >>> dpif_upcall_type); struct dpif_upcall { >>> /* All types. */ >>> enum dpif_upcall_type type; >>> - struct dp_packet packet; /* Packet data. */ >>> - struct nlattr *key; /* Flow key. */ >>> - size_t key_len; /* Length of 'key' in bytes. */ >>> - ovs_u128 ufid; /* Unique flow identifier for 'key'. */ >>> - struct nlattr *mru; /* Maximum receive unit. */ >>> - struct nlattr *cutlen; /* Number of bytes shrink from the end. */ >>> >>> /* DPIF_UC_ACTION only. */ >>> - struct nlattr *userdata; /* Argument to >> OVS_ACTION_ATTR_USERSPACE. */ >>> - struct nlattr *out_tun_key; /* Output tunnel key. */ >>> struct nlattr *actions; /* Argument to OVS_ACTION_ATTR_USERSPACE. >> */ >>> + struct nlattr *out_tun_key; /* Output tunnel key. */ >>> + struct nlattr *userdata; /* Argument to >> OVS_ACTION_ATTR_USERSPACE. */ >>> + >>> + struct nlattr *cutlen; /* Number of bytes shrink from the end. */ >>> + struct nlattr *mru; /* Maximum receive unit. */ >>> + ovs_u128 ufid; /* Unique flow identifier for 'key'. */ >>> + struct dp_packet packet; /* Packet data. */ >>> + struct nlattr *key; /* Flow key. */ >>> + size_t key_len; /* Length of 'key' in bytes. */ >>> }; >>> >>> /* A callback to notify higher layer of dpif about to be purged, so >>> that >>> -- >>> 2.4.11 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev >
>-----Original Message----- >From: Jarno Rajahalme [mailto:jarno@ovn.org] >Sent: Monday, October 10, 2016 9:01 PM >To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com> >Cc: dev@openvswitch.org >Subject: Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in dpif_upcall >structure. > >How about making the ‘dp_packet’ member the first member and adding a >comment that this should be first? This makes sense and by doing so we can avoid the hole, will make this change in V2. - Bhanu Prakash. > > Jarno > >> On Oct 10, 2016, at 8:42 AM, Bodireddy, Bhanuprakash ><bhanuprakash.bodireddy@intel.com> wrote: >> >> Hello jarno, >> >> Thanks for the feedback, while reordering the members of dpif_upcall, I had >to deviate from standards due to below reason. >> The dp_packet member has mbuf as first member that starts at new cache >line creating hole of size 60 bytes between dpif_upcall_type and dp_packet as >pointed below. >> >> struct dpif_upcall { >> enum dpif_upcall_type type; >> >> --> 60 bytes hole >> >> /* --- cacheline 1 boundary (64 bytes) --- */ >> struct dp_packet { >> struct rte_mbuf { >> /* typedef MARKER */ void * cacheline0[0]; /* 64 0 */ >> >> } >> struct nlattr * key; >> . >> . >> } >> >> I tried to pack this hole by moving other members in to this space. >> >> Regards, >> Bhanu Prakash. >> >>> -----Original Message----- >>> From: Jarno Rajahalme [mailto:jarno@ovn.org] >>> Sent: Friday, October 7, 2016 10:11 PM >>> To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com> >>> Cc: dev@openvswitch.org >>> Subject: Re: [ovs-dev] [PATCH 09/12] dpif: Reorder elements in >>> dpif_upcall structure. >>> >>> CodingStyle.md instructs to group struct members into related groups. >>> Also, changing the relative order of pointers should not make any >>> difference. Could you achieve the same by reordering just the members >>> above the ‘DPIF_UC_ACTION only.’ comment? >>> >>> Jarno >>> >>>> On Oct 7, 2016, at 9:17 AM, Bhanuprakash Bodireddy >>> <bhanuprakash.bodireddy@intel.com> wrote: >>>> >>>> By reordering the data elements in dpif_upcall structure, pad bytes >>>> can be reduced and also a cache line. >>>> >>>> Before: structure size:768, holes:1, sum padbytes:60, cachelines:12 >>>> After: structure size:656, holes:1, sum padbytes:4, cachelines:11 >>>> >>>> Signed-off-by: Bhanuprakash Bodireddy >>>> <bhanuprakash.bodireddy@intel.com> >>>> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com> >>>> --- >>>> lib/dpif.h | 17 +++++++++-------- >>>> 1 file changed, 9 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/lib/dpif.h b/lib/dpif.h index a7c5097..4a4bb3d 100644 >>>> --- a/lib/dpif.h >>>> +++ b/lib/dpif.h >>>> @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum >>>> dpif_upcall_type); struct dpif_upcall { >>>> /* All types. */ >>>> enum dpif_upcall_type type; >>>> - struct dp_packet packet; /* Packet data. */ >>>> - struct nlattr *key; /* Flow key. */ >>>> - size_t key_len; /* Length of 'key' in bytes. */ >>>> - ovs_u128 ufid; /* Unique flow identifier for 'key'. */ >>>> - struct nlattr *mru; /* Maximum receive unit. */ >>>> - struct nlattr *cutlen; /* Number of bytes shrink from the end. */ >>>> >>>> /* DPIF_UC_ACTION only. */ >>>> - struct nlattr *userdata; /* Argument to >>> OVS_ACTION_ATTR_USERSPACE. */ >>>> - struct nlattr *out_tun_key; /* Output tunnel key. */ >>>> struct nlattr *actions; /* Argument to >OVS_ACTION_ATTR_USERSPACE. >>> */ >>>> + struct nlattr *out_tun_key; /* Output tunnel key. */ >>>> + struct nlattr *userdata; /* Argument to >>> OVS_ACTION_ATTR_USERSPACE. */ >>>> + >>>> + struct nlattr *cutlen; /* Number of bytes shrink from the end. */ >>>> + struct nlattr *mru; /* Maximum receive unit. */ >>>> + ovs_u128 ufid; /* Unique flow identifier for 'key'. */ >>>> + struct dp_packet packet; /* Packet data. */ >>>> + struct nlattr *key; /* Flow key. */ >>>> + size_t key_len; /* Length of 'key' in bytes. */ >>>> }; >>>> >>>> /* A callback to notify higher layer of dpif about to be purged, so >>>> that >>>> -- >>>> 2.4.11 >>>> >>>> _______________________________________________ >>>> dev mailing list >>>> dev@openvswitch.org >>>> http://openvswitch.org/mailman/listinfo/dev >>
diff --git a/lib/dpif.h b/lib/dpif.h index a7c5097..4a4bb3d 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -779,17 +779,18 @@ const char *dpif_upcall_type_to_string(enum dpif_upcall_type); struct dpif_upcall { /* All types. */ enum dpif_upcall_type type; - struct dp_packet packet; /* Packet data. */ - struct nlattr *key; /* Flow key. */ - size_t key_len; /* Length of 'key' in bytes. */ - ovs_u128 ufid; /* Unique flow identifier for 'key'. */ - struct nlattr *mru; /* Maximum receive unit. */ - struct nlattr *cutlen; /* Number of bytes shrink from the end. */ /* DPIF_UC_ACTION only. */ - struct nlattr *userdata; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ - struct nlattr *out_tun_key; /* Output tunnel key. */ struct nlattr *actions; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ + struct nlattr *out_tun_key; /* Output tunnel key. */ + struct nlattr *userdata; /* Argument to OVS_ACTION_ATTR_USERSPACE. */ + + struct nlattr *cutlen; /* Number of bytes shrink from the end. */ + struct nlattr *mru; /* Maximum receive unit. */ + ovs_u128 ufid; /* Unique flow identifier for 'key'. */ + struct dp_packet packet; /* Packet data. */ + struct nlattr *key; /* Flow key. */ + size_t key_len; /* Length of 'key' in bytes. */ }; /* A callback to notify higher layer of dpif about to be purged, so that