RE: [PATCH net-next] net: mana: Add support for variable page sizes of ARM64

[Haiyang Zhang <haiyangz@microsoft.com>]

> -----Original Message-----
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Sent: Wednesday, June 12, 2024 10:22 AM
> To: Michael Kelley <mhklinux@outlook.com>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; Paul Rosswurm <paulros@microsoft.com>
> Cc: Dexuan Cui <decui@microsoft.com>; stephen@networkplumber.org; KY
> Srinivasan <kys@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; wei.liu@kernel.org;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; leon@kernel.org;
> Long Li <longli@microsoft.com>; ssengar@linux.microsoft.com; linux-
> rdma@vger.kernel.org; daniel@iogearbox.net; john.fastabend@gmail.com;
> bpf@vger.kernel.org; ast@kernel.org; hawk@kernel.org; tglx@linutronix.de;
> shradhagupta@linux.microsoft.com; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH net-next] net: mana: Add support for variable page
> sizes of ARM64
> 
> 
> 
> > -----Original Message-----
> > From: Michael Kelley <mhklinux@outlook.com>
> > Sent: Tuesday, June 11, 2024 10:42 PM
> > To: Haiyang Zhang <haiyangz@microsoft.com>; linux-
> hyperv@vger.kernel.org;
> > netdev@vger.kernel.org; Paul Rosswurm <paulros@microsoft.com>
> > Cc: Dexuan Cui <decui@microsoft.com>; stephen@networkplumber.org; KY
> > Srinivasan <kys@microsoft.com>; olaf@aepfle.de; vkuznets
> > <vkuznets@redhat.com>; davem@davemloft.net; wei.liu@kernel.org;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> leon@kernel.org;
> > Long Li <longli@microsoft.com>; ssengar@linux.microsoft.com; linux-
> > rdma@vger.kernel.org; daniel@iogearbox.net; john.fastabend@gmail.com;
> > bpf@vger.kernel.org; ast@kernel.org; hawk@kernel.org;
> tglx@linutronix.de;
> > shradhagupta@linux.microsoft.com; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH net-next] net: mana: Add support for variable page
> > sizes of ARM64
> 
> > > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > > b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > > index 1332db9a08eb..c9df942d0d02 100644
> > > > > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> > > > > @@ -182,7 +182,7 @@ int mana_gd_alloc_memory(struct gdma_context
> > *gc,
> > > > > unsigned int length,
> > > > >  dma_addr_t dma_handle;
> > > > >  void *buf;
> > > > >
> > > > > - if (length < PAGE_SIZE || !is_power_of_2(length))
> > > > > + if (length < MANA_MIN_QSIZE || !is_power_of_2(length))
> > > > >         return -EINVAL;
> > > > >
> > > > >  gmi->dev = gc->dev;
> > > > > @@ -717,7 +717,7 @@ EXPORT_SYMBOL_NS(mana_gd_destroy_dma_region,
> > NET_MANA);
> > > > >  static int mana_gd_create_dma_region(struct gdma_dev *gd,
> > > > >                          struct gdma_mem_info *gmi)
> > > > >  {
> > > > > - unsigned int num_page = gmi->length / PAGE_SIZE;
> > > > > + unsigned int num_page = gmi->length / MANA_MIN_QSIZE;
> > > >
> > > > This calculation seems a bit weird when using MANA_MIN_QSIZE. The
> > > > number of pages, and the construction of the page_addr_list array
> > > > a few lines later, seem unrelated to the concept of a minimum queue
> > > > size. Is the right concept really a "mapping chunk", and num_page
> > > > would conceptually be "num_chunks", or something like that?  Then
> > > > a queue must be at least one chunk in size, but that's derived from
> > the
> > > > chunk size, and is not the core concept.
> > >
> > > I think calling it "num_chunks" is fine.
> > > May I use "num_chunks" in next version?
> > >
> >
> > I think first is the decision on what to use for MANA_MIN_QSIZE. I'm
> > admittedly not familiar with mana and gdma, but the function
> > mana_gd_create_dma_region() seems fairly typical in defining a
> > logical region that spans multiple 4K chunks that may or may not
> > be physically contiguous.  So you set up an array of physical
> > addresses that identify the physical memory location of each chunk.
> > It seems very similar to a Hyper-V GPADL. I typically *do* see such
> > chunks called "pages", but a "mapping chunk" or "mapping unit"
> > is probably OK too.  So mana_gd_create_dma_region() would use
> > MANA_CHUNK_SIZE instead of MANA_MIN_QSIZE.  Then you could
> > also define MANA_MIN_QSIZE to be MANA_CHUNK_SIZE, for use
> > specifically when checking the size of a queue.
> >
> > Then if you are using MANA_CHUNK_SIZE, the local variable
> > would be "num_chunks".  The use of "page_count", "page_addr_list",
> > and "offset_in_page" field names in struct
> > gdma_create_dma_region_req should then be changed as well.
> 
> I'm fine with these names. I will check with Paul too.
> 
> I'd define just one macro, with a code comment like this. It
> will be used for chunk calculation and q len checking:
> /* Chunk size of MANA DMA, which is also the min queue size */
> #define MANA_CHUNK_SIZE 4096
> 
> 

After further discussion with Paul, and reading documents, we 
decided to use MANA_PAGE_SIZE for DMA unit calculations etc.
And use another macro MANA_MIN_QSIZE for queue length checking. 
This is also what Michael initially suggested. 

Thanks,
- Haiyang