From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Wei Hu <weh@microsoft.com>, "dev@dpdk.org" <dev@dpdk.org>,
Long Li <longli@microsoft.com>
Cc: "stable@dpdk.org" <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
Date: Tue, 19 Sep 2023 12:27:19 +0100 [thread overview]
Message-ID: <f372c13b-96db-4ab6-b14e-2be97de6e7d9@amd.com> (raw)
In-Reply-To: <SI2P153MB0441666EB252DB4AFD85FE07BBFAA@SI2P153MB0441.APCP153.PROD.OUTLOOK.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
>
next prev parent reply other threads:[~2023-09-19 11:27 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-09 12:23 Wei Hu
2023-09-13 21:11 ` Long Li
2023-09-14 5:11 ` Wei Hu
2023-09-18 18:02 ` Ferruh Yigit
2023-09-19 3:38 ` Wei Hu
2023-09-19 11:27 ` Ferruh Yigit [this message]
2023-09-20 3:11 ` Wei Hu
2023-09-18 20:01 ` Long Li
2023-09-19 2:13 ` Wei Hu
2023-09-19 19:23 ` Long Li
2023-09-20 8:10 ` Wei Hu
2023-09-20 17:28 ` Long Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f372c13b-96db-4ab6-b14e-2be97de6e7d9@amd.com \
--to=ferruh.yigit@amd.com \
--cc=bluca@debian.org \
--cc=dev@dpdk.org \
--cc=ktraynor@redhat.com \
--cc=longli@microsoft.com \
--cc=stable@dpdk.org \
--cc=weh@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).