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

[Pranjal Shrivastava <praan@google.com>]

On Mon, Nov 04, 2024 at 09:23:31AM -0800, Daniel Mentz wrote:
> On Thu, Oct 24, 2024 at 10:02 AM Pranjal Shrivastava <praan@google.com> wrote:
> >
> > 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:
> 
> Can you be more explicit about what is a good cacheline alignment? I'm
> wondering if you're trying to ensure that raw[4] is contained within a
> single cacheline as opposed to spanning two adjacent cachelines. I

I'm using a tool `pahole` [1] as suggested by Will earlier.
The tool prints information about the layout of structures, checking for
memory wastage due to padding and if the struct layout causes
mis-alignment with the caches.

The tool printed, "cacheline 1 boundary (64 bytes) was 24 bytes ago"
hinting that with the current layout, we are wasting 24-bytes of a
cacheline.

> doubt that this is worth optimizing for. Also, I'm wondering if this
> analysis assumes that the base address of a struct arm_smmu_event
> instance is cacheline aligned, which I am not sure is the case. I
> would solely optimize for size.

You're right, the tool does assume that the struct begins at a cacheline
I'm just sharing the analysis done by the tool to be able to finalize
the layout of `struct arm_smmu_event`.

IMO, if we don't have a strong opinion about this, there's no harm in
trying to layout the struct better even if it is a micro-optimization.

Although, I agree that size was the main concern here but if we have a
92-byte structure with 64-bit fields, we'll always have a 4-byte hole.

If we remove the `smmu` & `master_name` fields and pack all bools in
bitfields as discussed earlier, we'll have a size of 69 bytes,
(92 - 16 - 8 + 1), thus, we'd need a padding of 3 more bytes. Hence, the
size would be reduced to 72-bytes from 92-bytes. I guess, that's fine?

Thanks,
Praan