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