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

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/19/2023 4:38 AM, Wei Hu wrote:
> 
> 
>> -----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. 
> 

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: 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
>