Re: [PATCH v18 04/20] PCI: dwc: Change arguments of dw_pcie_prog_outbound_atu()

[Serge Semin <fancer.lancer@gmail.com>]

On Thu, Jul 27, 2023 at 04:33:43PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Jul 27, 2023 at 02:38:44AM +0300, Serge Semin wrote:
> > On Wed, Jul 26, 2023 at 06:30:15PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> > > > On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > > > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > > > To support INTx IRQs in the future, it requires an additional 2
> > > > > > arguments. For improved code readability, introduce the struct
> > > > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > > > dw_pcie_prog_outbound_atu().
> > > > > > 
> > > > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > > > a need.
> > > > > > 
> > > > > > No behavior changes.
> > > > > > 
> > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > > 
> > > > > One nit below. With that,
> > > > > 
> > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > 
> > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> > > > > > ---
> > > > > >  .../pci/controller/dwc/pcie-designware-ep.c   | 21 +++++---
> > > > > >  .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.c  | 49 ++++++-----------
> > > > > >  drivers/pci/controller/dwc/pcie-designware.h  | 15 ++++--
> > > > > >  4 files changed, 77 insertions(+), 60 deletions(-)
> > > > > > 
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > index 3c06e025c905..85de0d8346fa 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > > >  	DW_PCIE_NUM_CORE_RSTS
> > > > > >  };
> > > > > >  
> > > > > > +struct dw_pcie_ob_atu_cfg {
> > > > > > +	int index;
> > > > > > +	int type;
> > > > > > +	u8 func_no;
> > > > > > +	u64 cpu_addr;
> > > > > > +	u64 pci_addr;
> > > > > > +	u64 size;
> > > > > 
> > > > 
> > > > > Reorder the members in below order to avoid holes:
> > > > > 
> > > > > u64
> > > > > int
> > > > > u8
> > > > 
> > > > One more time. Your suggestion won't prevent the compiler from adding
> > > > the pads. (If by "holes" you meant the padding. Otherwise please
> > > > elaborate what you meant?).
> > > 
> > > Struct padding is often referred as struct holes. So yes, I'm referring the
> > > same.
> > > 
> > > > The structure will have the same size of
> > > > 40 bytes in both cases. So your suggestion will just worsen the
> > > > structure readability from having a more natural parameters order (MW
> > > > index, type, function, and then the mapping parameters) to a redundant
> > > > type-based order.
> > > > 
> > > 
> > 
> > > This is a common comment I provide for all structures. Even though the current
> > > result (reordering) doesn't save any space, when the structure grows big (who
> > > knows), we often see more holes/padding being inserted by the compiler if the
> > > members are not ordered in the descending order w.r.t their size.
> > > 
> > > I agree that it makes more clear if the members are grouped based on their
> > > function etc... but for large structures this would often add more padding/hole.
> > 
> > This structure will never be big enough to be considered for such
> > strange optimization. Moreover practicality almost always beats some
> > theoretical considerations. In this case there is no any reason to
> > reorder the fields as you say.
> > 
> > Speaking in general I very much doubt that saving a few bytes of
> > memory can be considered as a better option than having a more
> > readable structure especially these days. Moreover for all these years
> > I never met anybody asking to set the descending order of
> > the members or maintaining such limitation in the commonly used kernel
> > structures. What is normally done:
> > 1. Move an embedded object to the head of the structure for the
> > container_of-macro optimization.
> > 2. Group up the commonly used fields to optimize the system cache
> > utilization.
> > 3. Logical grouping the members, which naturally may lead to the more
> > optimal cache utilization.
> 
> Indeed.
> 
> > 4. Move a field to a certain place of the structure to fill in the
> > pads.
> > 
> 
> This is what I try to avoid by grouping the members. If you move a field to
> a certain place, wouldn't it affect readability?
> 
> But I do not want to argue more on this. Please see below.
> 
> > Even if the "descending alignment" requirement minimizes the number of
> > the pads it isn't the only possible way to do so in the particular
> > cases and it looks too harsh to be blindly applied all the time. If a
> > few bytes is so important why not do the same for instance for the
> > local variables too? They are also normally size-aligned in the stack
> > memory, which is much more precious in kernel.
> > 
> 
> Well, for local variables I prefer reverse Xmas tree order which is what widely
> used throughout the kernel. But we do not care about their ordering because, it
> won't grow too much like a structure (not talking about recursive case).
> 
> > Anyway in this case changing the fields order is absolutely redundant.
> > Even a provided afterwards update doesn't cause the structure size
> > change. So for the sake of readability it's better to leave its fields
> > ordered as is.
> > 
> 
> I certainly agree that reordering wouldn't save any space for this structure.
> As a maintainer, I prefer to keep this pattern so that I don't have to worry
> about the padding issues in the future and hence the suggestion.
> 
> But feel free to drop it as I don't have a strong objection to this specific
> case.

Agreed then.

Yoshihiro, could you please ignore the Mani' comment regarding the
"descending alignment" order and retain the fields order is in your
current patch?

I'll get back to the series review tomorrow. Please no rush with
resubmitting.

-Serge(y)

> 
> - Mani
> 
> > -Serge(y)
> > 
> > > 
> > > - Mani
> > > 
> > > > -Serge(y)
> > > > 
> > > > > 
> > > > > - Mani
> > > > > 
> > > > > > +};
> > > > > > +
> > > > > >  struct dw_pcie_host_ops {
> > > > > >  	int (*host_init)(struct dw_pcie_rp *pp);
> > > > > >  	void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > > >  int dw_pcie_link_up(struct dw_pcie *pci);
> > > > > >  void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > > >  int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > > -			      u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > -				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > > > +			      const struct dw_pcie_ob_atu_cfg *atu);
> > > > > >  int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > > >  			     u64 cpu_addr, u64 pci_addr, u64 size);
> > > > > >  int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > > > -- 
> > > > > > 2.25.1
> > > > > > 
> > > > > 
> > > > > -- 
> > > > > மணிவண்ணன் சதாசிவம்
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> -- 
> மணிவண்ணன் சதாசிவம்