Re: [PATCH v4 1/3] iommu/arm-smmu-v3: Introduce struct arm_smmu_event

[Pranjal Shrivastava <praan@google.com>]

On Thu, Oct 24, 2024 at 02:11:48PM +0100, Will Deacon wrote:
> On Fri, Oct 18, 2024 at 06:00:20PM +0000, Pranjal Shrivastava wrote:
> > +struct arm_smmu_event {
> > +	struct arm_smmu_device		*smmu;
> > +	u8				id;
> > +	u8				class;
> > +	u16				stag;
> > +	u32				sid;
> > +	u32				ssid;
> > +	u64				iova;
> > +	u64				ipa;
> > +	u64				raw[EVTQ_ENT_DWORDS];
> > +	bool				stall;
> > +	bool				ssid_valid;
> > +	bool				privileged;
> > +	bool				instruction;
> > +	bool				s2;
> > +	bool				read;
> > +};
> 
> minor nit, but it might be worth seeing what pahole says about the
> layout of this structure in case you've got a bunch of wasted padding
> thanks to the mixed-size fields.

I ran pahole with this, looks like there's only one 4-byte hole but the
cacheline aligment is bad (for a 64-byte cacheline):

struct arm_smmu_event {
	const char  *              master_name;          /*     0     8 */
	struct arm_smmu_device *   smmu;                 /*     8     8 */
	struct device *            dev;                  /*    16     8 */
	u8                         id;                   /*    24     1 */
	u8                         class;                /*    25     1 */
	u16                        stag;                 /*    26     2 */
	u32                        sid;                  /*    28     4 */
	u32                        ssid;                 /*    32     4 */

	/* XXX 4 bytes hole, try to pack */

	u64                        iova;                 /*    40     8 */
	u64                        ipa;                  /*    48     8 */
	u64                        raw[4];               /*    56    32 */
	/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
	bool                       stall;                /*    88     1 */
	bool                       ssid_valid;           /*    89     1 */
	bool                       privileged;           /*    90     1 */
	bool                       instruction;          /*    91     1 */
	bool                       s2;                   /*    92     1 */
	bool                       read;                 /*    93     1 */
	bool                       ttrnw;                /*    94     1 */
	bool                       ttrnw_valid;          /*    95     1 */

	/* size: 96, cachelines: 2, members: 19 */
	/* sum members: 92, holes: 1, sum holes: 4 */
	/* last cacheline: 32 bytes */
};

I don't think we can do much about the 4-byte hole as the members occupy
92 bytes only. I assume a single 4-byte hole shall be fine?

However, for cacheline aligment we can move the 3 top pointer-members,
`master_name`,`smmu` & `dev` which improves the cacheline aligment:

struct arm_smmu_event {
	u8                         id;                   /*     0     1 */
	u8                         class;                /*     1     1 */
	u16                        stag;                 /*     2     2 */
	u32                        sid;                  /*     4     4 */
	u32                        ssid;                 /*     8     4 */

	/* XXX 4 bytes hole, try to pack */

	u64                        iova;                 /*    16     8 */
	u64                        ipa;                  /*    24     8 */
	u64                        raw[4];               /*    32    32 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	bool                       stall;                /*    64     1 */
	bool                       ssid_valid;           /*    65     1 */
	bool                       privileged;           /*    66     1 */
	bool                       instruction;          /*    67     1 */
	bool                       s2;                   /*    68     1 */
	bool                       read;                 /*    69     1 */
	bool                       ttrnw;                /*    70     1 */
	bool                       ttrnw_valid;          /*    71     1 */
	const char  *              master_name;          /*    72     8 */
	struct arm_smmu_device *   smmu;                 /*    80     8 */
	struct device *            dev;                  /*    88     8 */

	/* size: 96, cachelines: 2, members: 19 */
	/* sum members: 92, holes: 1, sum holes: 4 */
	/* last cacheline: 32 bytes */
};

I'll fix this in the next version of the patch. Thanks!

> 
> Will

Thanks,
Praan