From: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
To: fengchengwen <fengchengwen@huawei.com>,
Amit Prakash Shukla <amitprakashs@marvell.com>,
Cheng Jiang <honest.jiang@foxmail.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Jerin Jacob <jerinj@marvell.com>,
Anoob Joseph <anoobj@marvell.com>,
Kevin Laatz <kevin.laatz@intel.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
Subject: RE: [EXTERNAL] Re: [EXT] Re: [PATCH v10 4/4] app/dma-perf: add SG copy support
Date: Fri, 1 Mar 2024 08:06:08 +0000 [thread overview]
Message-ID: <CO1PR18MB4714BFAFDF56393699055E8DCB5E2@CO1PR18MB4714.namprd18.prod.outlook.com> (raw)
In-Reply-To: <3fa6868b-2d6d-9c09-c993-aa7ace1c7d80@huawei.com>
Hi Fengcheng,
<cut>
> >>> -output_result(uint8_t scenario_id, uint32_t lcore_id, char
> >>> *dma_name,
> >> uint16_t ring_size,
> >>> - uint16_t kick_batch, uint64_t ave_cycle, uint32_t
> >> buf_size, uint32_t nr_buf,
> >>> - float memory, float bandwidth, float mops, bool
> >> is_dma)
> >>> +output_result(struct test_configure *cfg, struct lcore_params *para,
> >>> + uint16_t kick_batch, uint64_t ave_cycle, uint32_t
> >> buf_size,
> >>> + uint32_t nr_buf, float memory, float bandwidth, float
> >> mops)
> >>> {
> >>> - if (is_dma)
> >>> - printf("lcore %u, DMA %s, DMA Ring Size: %u, Kick Batch Size:
> >> %u.\n",
> >>> - lcore_id, dma_name, ring_size, kick_batch);
> >>> - else
> >>> + uint16_t ring_size = cfg->ring_size.cur;
> >>> + uint8_t scenario_id = cfg->scenario_id;
> >>> + uint32_t lcore_id = para->lcore_id;
> >>> + char *dma_name = para->dma_name;
> >>> +
> >>> + if (cfg->is_dma) {
> >>> + printf("lcore %u, DMA %s, DMA Ring Size: %u, Kick Batch Size:
> >> %u", lcore_id,
> >>> + dma_name, ring_size, kick_batch);
> >>> + if (cfg->is_sg)
> >>> + printf(" DMA src ptrs: %u, dst ptrs: %u",
> >>> + para->src_ptrs, para->dst_ptrs);
> >>
> >> DMA src sges: %u DMA dst sges: %u
> >>
> >> I think we should add a column which title maybe misc, some like
> >> sg-src[4]- dst[1], and later we may add fill test, then this field
> >> could be pattern-
> >> 0x12345678
> >>
> >> And in "[PATCH v10 2/4] app/dma-perf: add PCI device support" commit,
> >> if the DMA was worked in non-mem2mem direction, we could add simple
> >> descriptor of direction and pcie.info in the above misc column.
> >>
> >
> > I am sorry, I could not understand complete picture here. Do you mean
> > we reserve a column and use it as per test type.
> >
> > For plain mem copy, nothing added.
> > For SG mem copy, instead of showing "DMA src sges: 1, dst sges: 4", print
> "sg-src[1]-dst[4]".
> > In future, when we add fill test in benchmark, this line instead be "pattern-
> 0x12345678".
> >
> > Is my understanding correct over here ?
>
> Yes, some like this.
>
This patch adds SGE info in an alignment with existing output.
I think it is better to add further extensions as we add new features. Since the app doesn't support the features that you mentioned, it is difficult to anticipate the requirements.
In fact, if the additional frameworks that we put in are not useful for those features, it could lead to stale code.
I would prefer if we can make these changes as we add new features.
> >
<cut>
> >>> }
> >>>
> >>> while (1) {
> >>> @@ -541,13 +702,53 @@ mem_copy_benchmark(struct test_configure
> >> *cfg, bool is_dma)
> >>>
> >>> rte_eal_mp_wait_lcore();
> >>>
> >>> - for (i = 0; i < (nr_buf / nb_workers) * nb_workers; i++) {
> >>> - if (memcmp(rte_pktmbuf_mtod(srcs[i], void *),
> >>> - rte_pktmbuf_mtod(dsts[i], void *),
> >>> - cfg->buf_size.cur) != 0) {
> >>> - printf("Copy validation fails for buffer number %d\n",
> >> i);
> >>> - ret = -1;
> >>> - goto out;
> >>> + if (!cfg->is_sg && cfg->transfer_dir == RTE_DMA_DIR_MEM_TO_MEM)
> >> {
> >>> + for (i = 0; i < (nr_buf / nb_workers) * nb_workers; i++) {
> >>> + if (memcmp(rte_pktmbuf_mtod(srcs[i], void *),
> >>> + rte_pktmbuf_mtod(dsts[i], void *),
> >>> + cfg->buf_size.cur) != 0) {
> >>> + printf("Copy validation fails for buffer number
> >> %d\n", i);
> >>> + ret = -1;
> >>> + goto out;
> >>> + }
> >>> + }
> >>> + } else if (cfg->is_sg && cfg->transfer_dir ==
> >> RTE_DMA_DIR_MEM_TO_MEM) {
> >>> + size_t src_remsz = buf_size % cfg->src_ptrs;
> >>> + size_t dst_remsz = buf_size % cfg->dst_ptrs;
> >>> + size_t src_sz = buf_size / cfg->src_ptrs;
> >>> + size_t dst_sz = buf_size / cfg->dst_ptrs;
> >>> + uint8_t src[buf_size], dst[buf_size];
> >>> + uint8_t *sbuf, *dbuf, *ptr;
> >>> +
> >>> + for (i = 0; i < (nr_buf / RTE_MAX(cfg->src_ptrs, cfg->dst_ptrs));
> >> i++) {
> >>> + sbuf = src;
> >>> + dbuf = dst;
> >>> + ptr = NULL;
> >>> +
> >>> + for (j = 0; j < cfg->src_ptrs; j++) {
> >>> + ptr = rte_pktmbuf_mtod(srcs[i * cfg->src_ptrs
> >> + j], uint8_t *);
> >>> + memcpy(sbuf, ptr, src_sz);
> >>> + sbuf += src_sz;
> >>> + }
> >>> +
> >>> + if (src_remsz)
> >>> + memcpy(sbuf, ptr + src_sz, src_remsz);
> >>> +
> >>> + for (j = 0; j < cfg->dst_ptrs; j++) {
> >>> + ptr = rte_pktmbuf_mtod(dsts[i * cfg->dst_ptrs
> >> + j], uint8_t *);
> >>> + memcpy(dbuf, ptr, dst_sz);
> >>> + dbuf += dst_sz;
> >>> + }
> >>> +
> >>> + if (dst_remsz)
> >>> + memcpy(dbuf, ptr + dst_sz, dst_remsz);
> >>> +
> >>> + if (memcmp(src, dst, buf_size) != 0) {
> >>> + printf("SG Copy validation fails for buffer
> >> number %d\n",
> >>> + i * cfg->src_ptrs);
> >>> + ret = -1;
> >>> + goto out;
> >>> + }
> >>
> >> Now I doubt the value of verify, this verify can't find the middle
> >> round copy failure, because as long as the last round copy is
> >> successful, the validation will pass.
> >>
> > Validation is on entire buffer. If any middle copy is a failure,
> > entire memcmp would have failed. Or do I miss something ?
> >
> >> And adding validatation in every round copy will impact performance.
> >>
> > This validation is just after worker function is stopped measuring perf.
> > How would this impact performance ?
>
> Yes, it will don't impact performance.
>
> What I said before is that is not valid, pls consider following scene:
>
>
> while (1) {
> for (i = 0; i < nr_buf; i++) { // this for loop will copy all nr_bufs,
> let's defind this is a round copy.
> dma_copy:
> ret = rte_dma_copy(dev_id, 0,
> rte_mbuf_data_iova(srcs[i]),
> rte_mbuf_data_iova(dsts[i]), buf_size, 0);
> if (unlikely(ret < 0)) {
> if (ret == -ENOSPC) {
> do_dma_submit_and_poll(dev_id,
> &async_cnt, worker_info);
> goto dma_copy;
> } else
> error_exit(dev_id);
> }
> async_cnt++;
>
> if ((async_cnt % kick_batch) == 0)
> do_dma_submit_and_poll(dev_id,
> &async_cnt, worker_info);
> }
>
> if (worker_info->stop_flag) // if don't stop, it will do many
> round copies.
> break;
> }
>
> and the later validation just verify the last round, let's assume there are 100
> round, and if the last round copy work well, but round 0~98 both copy fail,
> then the validation will not detect it.
>
>
> So if we want do all the validation, then we should add the velidation after
> every round copy, but it will impact the performance.
>
>
> >
> >> Also app/test_dmadev already verify data. so I think we should drop
> >> the validation commit.
> >
> > Even in some corner cases or unknown issues, copy would have failed
> > and taking perf cycles then is meaningless. That is the reason, this
> > validation is added after perf function doing its job.
>
> How about:
>
> while (1) {
> for (i = 0; i < nr_buf; i++) { // this for loop will copy all nr_bufs,
> let's defind this is a round copy.
> dma_copy:
> ret = rte_dma_copy(dev_id, 0,
> rte_mbuf_data_iova(srcs[i]),
> rte_mbuf_data_iova(dsts[i]), buf_size, 0);
> if (unlikely(ret < 0)) {
> if (ret == -ENOSPC) {
> do_dma_submit_and_poll(dev_id,
> &async_cnt, worker_info);
> goto dma_copy;
> } else
> error_exit(dev_id);
> }
> async_cnt++;
>
> if ((async_cnt % kick_batch) == 0)
> do_dma_submit_and_poll(dev_id,
> &async_cnt, worker_info);
> }
>
> if (unlikely(work_info->verify)) {
> ret = verify();
> if (ret != 0) {
> // error trace,
> break;
> }
> }
>
> if (worker_info->stop_flag) // if don't stop, it will do many
> round copies.
> break;
> }
>
> and make this verify as a config entry
I believe there is a difference in understanding of what this is intended to do. Intention here is not to validate every operation done by DMA, and that is already taken care by UT.
Is it possible that we are we misreporting numbers if the application is buggy or PMD is misbehaving for the scenario under test and the copies are not actually performed? Yes. Think about a scenario where PMD is buggy when trying bursts of more than 1.
Checking last set of buffers is more like testing a sample from the perf test to make sure perf test was indeed performing what it is claiming to do. If you think it is unnecessary to do so, we can drop this from upstream. But adding complete verification in performance app would be repeating what a unit test is expected to do. I would suggest not to do that.
Thanks,
Gowrishankar
next prev parent reply other threads:[~2024-03-01 8:06 UTC|newest]
Thread overview: 79+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-10 10:57 [PATCH v2] " Gowrishankar Muthukrishnan
2023-08-10 13:01 ` [PATCH v3 0/2] " Gowrishankar Muthukrishnan
2023-08-10 13:01 ` [PATCH v3 1/2] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2023-08-23 11:46 ` [EXT] " Pavan Nikhilesh Bhagavatula
2023-08-10 13:01 ` [PATCH v3 2/2] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2023-09-21 3:02 ` [PATCH v3 0/2] " Jiang, Cheng1
2023-09-24 9:32 ` [PATCH v4 " Gowrishankar Muthukrishnan
2023-09-24 9:32 ` [PATCH v4 1/2] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2023-09-24 9:32 ` [PATCH v4 2/2] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2023-09-28 21:12 ` Pavan Nikhilesh Bhagavatula
2023-10-26 18:31 ` [PATCH v5 0/4] app/dma-perf: PCI Dev and " Gowrishankar Muthukrishnan
2023-10-26 18:31 ` [PATCH v5 1/4] app/dma-perf: add skip support Gowrishankar Muthukrishnan
2023-11-10 9:03 ` Anoob Joseph
2023-10-26 18:31 ` [PATCH v5 2/4] app/dma-perf: add PCI device support Gowrishankar Muthukrishnan
2023-11-10 9:04 ` Anoob Joseph
2023-10-26 18:31 ` [PATCH v5 3/4] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2023-11-10 9:05 ` Anoob Joseph
2023-10-26 18:31 ` [PATCH v5 4/4] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2023-11-10 9:07 ` Anoob Joseph
2023-11-13 4:41 ` [PATCH v6 0/4] PCI Dev and " Gowrishankar Muthukrishnan
2023-11-13 4:41 ` [PATCH v6 1/4] app/dma-perf: add skip support Gowrishankar Muthukrishnan
2023-11-13 4:41 ` [PATCH v6 2/4] app/dma-perf: add PCI device support Gowrishankar Muthukrishnan
2023-11-13 4:41 ` [PATCH v6 3/4] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2023-11-13 4:41 ` [PATCH v6 4/4] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2023-11-17 12:15 ` [PATCH v7 0/4] PCI Dev and " Gowrishankar Muthukrishnan
2023-11-17 12:15 ` [PATCH v7 1/4] app/dma-perf: add skip support Gowrishankar Muthukrishnan
2023-11-20 2:54 ` fengchengwen
2023-11-22 12:01 ` [EXT] " Amit Prakash Shukla
2023-11-17 12:15 ` [PATCH v7 2/4] app/dma-perf: add PCI device support Gowrishankar Muthukrishnan
2023-11-17 12:15 ` [PATCH v7 3/4] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2023-11-17 12:15 ` [PATCH v7 4/4] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2023-11-22 11:06 ` [PATCH v8 0/4] PCI Dev and " Gowrishankar Muthukrishnan
2023-11-22 11:06 ` [PATCH v8 1/4] app/dma-perf: add skip support Gowrishankar Muthukrishnan
2023-11-22 11:06 ` [PATCH v8 2/4] app/dma-perf: add PCI device support Gowrishankar Muthukrishnan
2023-11-23 1:12 ` fengchengwen
2024-02-21 3:26 ` fengchengwen
2024-02-27 9:27 ` [EXT] " Amit Prakash Shukla
2023-11-22 11:06 ` [PATCH v8 3/4] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2023-11-23 1:14 ` fengchengwen
2023-11-22 11:06 ` [PATCH v8 4/4] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2024-01-25 12:44 ` fengchengwen
2024-02-21 3:52 ` fengchengwen
2024-02-27 16:09 ` [EXT] " Gowrishankar Muthukrishnan
2023-12-07 10:11 ` [PATCH v8 0/4] PCI Dev and " Gowrishankar Muthukrishnan
2024-02-05 10:37 ` Gowrishankar Muthukrishnan
2024-02-27 16:00 ` [PATCH v9 " Amit Prakash Shukla
2024-02-27 16:00 ` [PATCH v9 1/4] app/dma-perf: add skip support Amit Prakash Shukla
2024-02-27 16:00 ` [PATCH v9 2/4] app/dma-perf: add PCI device support Amit Prakash Shukla
2024-02-27 16:00 ` [PATCH v9 3/4] app/dma-perf: validate copied memory Amit Prakash Shukla
2024-02-27 16:00 ` [PATCH v9 4/4] app/dma-perf: add SG copy support Amit Prakash Shukla
2024-02-27 18:35 ` [PATCH v10 0/4] PCI Dev and " Amit Prakash Shukla
2024-02-27 18:35 ` [PATCH v10 1/4] app/dma-perf: add skip support Amit Prakash Shukla
2024-02-27 18:35 ` [PATCH v10 2/4] app/dma-perf: add PCI device support Amit Prakash Shukla
2024-02-27 18:35 ` [PATCH v10 3/4] app/dma-perf: validate copied memory Amit Prakash Shukla
2024-02-28 8:10 ` fengchengwen
2024-02-28 9:09 ` [EXT] " Gowrishankar Muthukrishnan
2024-02-29 13:48 ` [v11 0/4] PCI Dev and SG copy support Gowrishankar Muthukrishnan
2024-02-29 13:48 ` [v11 1/4] app/dma-perf: add skip support Gowrishankar Muthukrishnan
2024-02-29 13:48 ` [v11 2/4] app/dma-perf: add PCI device support Gowrishankar Muthukrishnan
2024-02-29 13:48 ` [v11 3/4] app/dma-perf: validate copied memory Gowrishankar Muthukrishnan
2024-02-29 13:48 ` [v11 4/4] app/dma-perf: add SG copy support Gowrishankar Muthukrishnan
2024-03-06 19:50 ` [v11 0/4] PCI Dev and " Thomas Monjalon
2024-03-07 13:48 ` fengchengwen
2024-03-07 13:55 ` [EXTERNAL] " Gowrishankar Muthukrishnan
2024-03-12 9:15 ` Thomas Monjalon
2024-03-12 12:05 ` fengchengwen
2024-03-12 12:24 ` Gowrishankar Muthukrishnan
2024-03-13 7:26 ` fengchengwen
2024-03-13 8:22 ` Gowrishankar Muthukrishnan
2024-03-15 7:30 ` Gowrishankar Muthukrishnan
2024-03-15 13:09 ` Thomas Monjalon
2024-03-18 7:32 ` Gowrishankar Muthukrishnan
2024-03-07 13:48 ` Gowrishankar Muthukrishnan
2024-02-27 18:56 ` [PATCH v10 4/4] app/dma-perf: add " Amit Prakash Shukla
2024-02-28 9:31 ` fengchengwen
2024-02-29 13:16 ` [EXT] " Gowrishankar Muthukrishnan
2024-03-01 2:07 ` fengchengwen
2024-03-01 8:06 ` Gowrishankar Muthukrishnan [this message]
2024-03-01 9:45 ` [EXTERNAL] " fengchengwen
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=CO1PR18MB4714BFAFDF56393699055E8DCB5E2@CO1PR18MB4714.namprd18.prod.outlook.com \
--to=gmuthukrishn@marvell.com \
--cc=amitprakashs@marvell.com \
--cc=anoobj@marvell.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=honest.jiang@foxmail.com \
--cc=jerinj@marvell.com \
--cc=kevin.laatz@intel.com \
--cc=pbhagavatula@marvell.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).