RE: [PATCH v2 4/8] net/rnp: add mbx basic api feature

["11" <caowenbo@mucse.com>]

Hello, Stephen
Fort this struct is used for communicate with firmware or
Pf communicate with vf.
It is used for sync the communicative event.
It isn't used for performance sampling.
For u32(uint32_t) is because of the hardware register is 32bit read/write.
So this counter size is enouth.
>+struct rnp_mbx_stats {
>...
>+}

Regards Wenbo
-----Original Message-----
From: Stephen Hemminger <stephen@networkplumber.org> 
Sent: 2023年8月3日 0:12
To: Wenbo Cao <caowenbo@mucse.com>
Cc: dev@dpdk.org; ferruh.yigit@intel.com; andrew.rybchenko@oktetlabs.ru
Subject: Re: [PATCH v2 4/8] net/rnp: add mbx basic api feature

On Wed,  2 Aug 2023 08:11:02 +0000
Wenbo Cao <caowenbo@mucse.com> wrote:

> +struct rnp_mbx_stats {
> +	u32 msgs_tx;
> +	u32 msgs_rx;
> +
> +	u32 acks;
> +	u32 reqs;
> +	u32 rsts;
> +};

Easy to cause wraparound of 32 bits in these stats.
Why use u32 and not uint32_t or uint64_t.

> +struct rnp_mbx_info {
> +	struct rnp_mbx_api ops;
> +	uint32_t usec_delay;    /* retry interval delay time */
> +	uint32_t timeout;       /* retry ops timeout limit */
> +	uint16_t size;          /* data buffer size*/
> +	uint16_t vf_num;        /* Virtual Function num */
> +	uint16_t pf_num;        /* Physical Function num */
> +	uint16_t sriov_st;      /* Sriov state */
> +	bool irq_enabled;
> +	union {
> +		struct {
> +			unsigned short pf_req;
> +			unsigned short pf_ack;
> +		};
> +		struct {
> +			unsigned short cpu_req;
> +			unsigned short cpu_ack;
> +		};
> +	};
> +	unsigned short vf_req[64];
> +	unsigned short vf_ack[64];
> +
> +	struct rnp_mbx_stats stats;
> +
> +	rte_atomic16_t state;
> +} __rte_cache_aligned;

You may want to reorder this data structure to reduce cache misses.
The last element is almost certainly on another cache line.
Also, if you want faster statistics should consider per core, per queue or
per VF.