[ovs-dev] [PATCH v7] Detailed packet drop statistics per dpdk and vhostuser ports

Ilya Maximets i.maximets at samsung.com
Fri Sep 6 13:17:59 UTC 2019


On 05.09.2019 9:58, Sriram Vatala wrote:
> Hi Ilya,
> Thanks a lot for the explanation. As per your suggestion, I will move all the 
> counters (including 'tx_retries')to some structure and place a pointer to it 
> in netdev_dpdk structure so that the padding size will not vary with the 
> introduction of new counters in future.
> 
> @Kevin Traynor : I will change the comment line for the number of padding 
> bytes in my next patch instead of you sending a new patch just for changing 
> the comment line.
> 
> Thanks & Regards,
> Sriram.
> 
> -----Original Message-----
> From: Ilya Maximets <i.maximets at samsung.com>
> Sent: 04 September 2019 19:34
> To: Sriram Vatala <sriram.v at altencalsoftlabs.com>; Kevin Traynor 
> <ktraynor at redhat.com>
> Cc: ovs-dev at openvswitch.org; Stokes, Ian <ian.stokes at intel.com>
> Subject: Re: [PATCH v7] Detailed packet drop statistics per dpdk and vhostuser 
> ports
> 
> On 04.09.2019 16:31, Sriram Vatala wrote:
>> Hi Ilya,
>> 1) I was working on addressing the comments provided by you. Had a
>> small query on one of your comments.
>> 2) I am trying to understand the problem of padding bytes in struct
>> netdev_dpdk which you are referring to in your comment.
>> 3) If I understand correctly, the macro "PADDED_MEMBERS(UNIT, MEMBERS)"
>> ensures that the memory to be allocated for all 'MEMBERS' is rounded
>> off to nearest multiple of CACHE_LINE_SIZE which  is 64 in this case.
>> This macro adds pad bytes to roundoff to multiple of 64.
>> 4) At runtime, I checked the size of stats section of netdev_dpdk
>> without new counters that I have introduced in my patch. It is 384
>> bytes, out of which struct netdev_stats alone occupies 336 bytes and 8
>> bytes for tx_retries counter. (I could see there is no padding between
>> netdev_stats and tx_retries). Total of 344 is rounded to 384 [((344 + 64 - 
>> 1)/64) * 64].
>> 5) With my new counters, the size remains same after padding.
>> (336 + 8 + 8 +8 +8 +8 = 376) which is rounded to 384 bytes  [((376 +64
>> -1)/64) *64]  at runtime.
>>
>> I want to check with you, if I have correctly understood the problem
>> that you are referring in your comment. If you can explain a bit more
>> on this, it would be helpful for me to understand the problem and think of 
>> possible solutions.
>>
>> Following is the snippet of memory layout of netdev_dpdk at runtime :
>>     union {
>>         OVS_CACHE_LINE_MARKER cacheline1;
>>         struct {...};
>>         uint8_t pad50[64];
>>     };
>>     union {
>>         struct {...};
>>         uint8_t pad51[192];
>>     };
>>     union {
>>         struct {...};
>>         uint8_t pad52[384];
>>     };
>>     union {
>>         struct {...};
>>         uint8_t pad53[128];
>>     };
>>     union {
>>         struct {...};
>>         uint8_t pad54[64];
>>     };
>> }
> 
> Hi.
> 
> The code looks like this:
> 
>      PADDED_MEMBERS(CACHE_LINE_SIZE,
>          struct netdev_stats stats;
>          /* Custom stat for retries when unable to transmit. */
>          uint64_t tx_retries;
>          /* Protects stats */
>          rte_spinlock_t stats_lock;
>          /* 4 pad bytes here. */    <-- This comment I'm talking about.
>      );
> 
> The only thing you need to change is to update the comment while adding new 
> structure fields.
> 
> Your calculations are missing the size of 'stats_lock' which is 4 bytes.
> So, on current master total size is:
>     336 bytes for stats + 8 bytes for tx_retries + 4 bytes for lock = 352 And 
> it's rounded up to 384 by padding, i.e. we have 384 - 352 = 32 pad bytes.

Sorry, I miscalculated this too.)
336 + 8 + 4 = 348
pahole reports 352, because there is an additional 4 bytes padding inside the
unnamed structure.


> The comment on current master should be "/* 32 pad bytes here. */".

So, the comment should be "/* 36 pad bytes here. */".

> 
> BTW, Kevin, how did you calculate 4 here? My pahole output is following:
> 
>         union {
>                 struct {
>                         struct netdev_stats stats;       /*   320   336 */
>                         uint64_t   tx_retries;           /*   656     8 */
>                         rte_spinlock_t stats_lock;       /*   664     4 */
>                 };                                       /*         352 */
>                 uint8_t            pad52[0];             /*           0 */
>         };                                               /*   320   384 */
> 
> Returning to the patch. After adding 4 more stats (4 * 8 bytes), the new 
> layout
> takes:
>    336 bytes for stats + 8 bytes for tx_retries \
>    + 4*8 bytes for new stats + 4 bytes for lock = 384 bytes.
> 
> Now the structure takes exactly 384 bytes and you need to remove the comment 
> or change it to "/* 0 pad bytes here. */".

And 4 bytes will remain in padding after adding new stats.

> 
> Sorry, I didn't check the actual layout until now so I was confused a bit by 
> the comment on current master. Anyway, you need to update that comment.
> However, It might be still useful to move these stats to a separate structure 
> to avoid big padding in case we'll want to add one more. And I'm still 
> thinking that we need to drop paddings at all for most of the structure 
> fields, but this should be a separate change.
> 
> Best regards, Ilya Maximets.
> 


More information about the dev mailing list