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.maxim...@samsung.com>
> Sent: 04 September 2019 19:34
> To: Sriram Vatala <srira...@altencalsoftlabs.com>; Kevin Traynor
> <ktray...@redhat.com>
> Cc: ovs-dev@openvswitch.org; Stokes, Ian <ian.sto...@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.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
