From: Wei Hu <weh@microsoft.com>
To: Ferruh Yigit <ferruh.yigit@amd.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 03:38:32 +0000 [thread overview]
Message-ID: <SI2P153MB0441666EB252DB4AFD85FE07BBFAA@SI2P153MB0441.APCP153.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <9f6cc71e-b5d3-4d9f-9ca8-37baf839017e@amd.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
next prev parent reply other threads:[~2023-09-19 3:38 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 [this message]
2023-09-19 11:27 ` Ferruh Yigit
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=SI2P153MB0441666EB252DB4AFD85FE07BBFAA@SI2P153MB0441.APCP153.PROD.OUTLOOK.COM \
--to=weh@microsoft.com \
--cc=bluca@debian.org \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=ktraynor@redhat.com \
--cc=longli@microsoft.com \
--cc=stable@dpdk.org \
/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).