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.
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
