RE: [PATCH 1/1] net/mana: add 32 bit short doorbell

[Wei Hu <weh@microsoft.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Tuesday, September 19, 2023 2:03 AM
> To: Wei Hu <weh@microsoft.com>; dev@dpdk.org; Long Li
> <longli@microsoft.com>
> Cc: stable@dpdk.org; Kevin Traynor <ktraynor@redhat.com>; Luca Boccassi
> <bluca@debian.org>
> Subject: Re: [PATCH 1/1] net/mana: add 32 bit short doorbell
> 
> On 9/9/2023 1:23 PM, Wei Hu wrote:
> > Add 32 bit short doorbell support. Ring short doorbell when running in
> > 32 bit applicactions.
> >
> 
> Hi Wei,
> 
> Is this performance improvement for 32 bit, or is short doorbell support
> required for 32 bit support?
> 	
> 
Hi Ferruh,

This it not a performance improvement patch. Without this patch, 32 bit applications
do not function. 

> This patch is using RTE_ARCH_32 compile time macro to enable short doorbell
> support, so need to decide to support 32 bit or 64 bit in compile time.
> 
> Also I guess 32 bit driver can run on 64 bit arch, what will be the result in that
> case?

The patch is for those applications compiled in 32 bit, but running on a 64 bit Linux
kernels.  There is no 64 bit mana kernel driver available. So the kernel still needs to be
in 64 bit.
> 
> My point is, instead of using compile time flag, what do you think to detect
> execution platform on runtime and use preferred doorbell according platform?
> 
> I can see short descriptor support touches multiple functions, can the support
> be abstracted to let to use it based on runtime detection?

The 32 bit support request is from a specific customer who only has 32 bit applications. 
The customer needs to build and link its applications into 32bit libraries and drivers. 
Therefore, the DPDK mana driver needs to be in 32 bit anyway. 

32bit applications cannot use 64bit doorbells. 64bit applications can use 32bit doorbells,
however the performance would suffer and it definitely not recommended. 

IMHO, there is not much difference between compile time flag and "if...then...else" statement
at runtime, except for in the latter case, a few more extra runtime instructions and maybe
some branch overhead in either 64bit or 32bit case.  Given we have limited 32bit use
cases, I chose to just use the compile time flag, which seems to be simpler to implement
and less work for our verification team. 

> 
> > Cc: stable@dpdk.org
> >
> 
> Similar comment as previous patch, this patch is not a fix but adding new
> support, not sure about backporting it.
> 
The customer who needs 32bit support wants it to be on 22.11.x. That is why 
I put "Cc: stable@dpdk.org" in it.

> > Signed-off-by: Wei Hu <weh@microsoft.com>
> >
> 
> <...>
> 
> > @@ -97,6 +110,7 @@ mana_alloc_and_post_rx_wqe(struct mana_rxq *rxq)
> >  		/* update queue for tracking pending packets */
> >  		desc->pkt = mbuf;
> >  		desc->wqe_size_in_bu = wqe_size_in_bu;
> > +		rxq->wqe_cnt_to_short_db += wqe_size_in_bu;
> >
> 
> This variable always used within RTE_ARCH_32 block, but set here without
> RTE_ARCH_32 ifdef, is this intentional?

No, it is not intentional. Thanks for pointing this out. I will add ifdef in next
round.

Thanks,
Wei