On 9/19/2023 4:38 AM, Wei Hu wrote: > > >> -----Original Message----- >> From: Ferruh Yigit <ferruh.yi...@amd.com> >> Sent: Tuesday, September 19, 2023 2:03 AM >> To: Wei Hu <w...@microsoft.com>; dev@dpdk.org; Long Li >> <lon...@microsoft.com> >> Cc: sta...@dpdk.org; Kevin Traynor <ktray...@redhat.com>; Luca Boccassi >> <bl...@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. >
OK >> 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. > OK, can you please add this detail to the commit log? > 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. > You are right, I was thinking it can help on deployment, but need to compile separate binaries for 32bit and 64bit anyway, so not much gain. >> >>> Cc: sta...@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: sta...@dpdk.org" in it. > >>> Signed-off-by: Wei Hu <w...@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 >
