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

Kevin Traynor ktraynor at redhat.com
Wed Sep 4 15:54:33 UTC 2019


On 04/09/2019 15:03, Ilya Maximets wrote:
> 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.
> The comment on current master should be "/* 32 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 */
> 

Hmm, looks like I must have misinterpreted. I will send a patch for
branch-2.12 to update the comment. Not sure it's worth to send a patch
for master as this patch is changing it anyway, but I can if there is a
preference.

> 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. */".
> 
> 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