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

Sriram Vatala sriram.v at altencalsoftlabs.com
Thu Sep 5 06:58:00 UTC 2019


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.
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.


More information about the dev mailing list