DPDK patches and discussions
 help / color / mirror / Atom feed
From: fengchengwen <fengchengwen@huawei.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: <dev@dpdk.org>, <mb@smartsharesystems.com>,
	Kevin Laatz <kevin.laatz@intel.com>,
	Bruce Richardson <bruce.richardson@intel.com>,
	<david.marchand@redhat.com>
Subject: Re: [PATCH v4] dmadev: add tracepoints
Date: Tue, 7 Nov 2023 09:26:38 +0800	[thread overview]
Message-ID: <36c44f2f-fb5d-e23b-6289-c0a908aede43@huawei.com> (raw)
In-Reply-To: <2896474.SvYEEZNnvj@thomas>

Hi Thomas,

On 2023/11/7 4:59, Thomas Monjalon wrote:
> 11/10/2023 11:55, fengchengwen:
>> Hi Thomas,
>>
>>   Sorry for the late reply.
>>
>> On 2023/8/14 22:16, Thomas Monjalon wrote:
>>> jeudi 3 août 2023, fengchengwen:
>>>> Hi Thomas,
>>>>
>>>> On 2023/7/31 20:48, Thomas Monjalon wrote:
>>>>> 10/07/2023 09:50, fengchengwen:
>>>>>> Hi Thomas,
>>>>>>
>>>>>> On 2023/7/10 14:49, Thomas Monjalon wrote:
>>>>>>> 09/07/2023 05:23, fengchengwen:
>>>>>>>> Hi Thomas,
>>>>>>>>
>>>>>>>> On 2023/7/7 18:40, Thomas Monjalon wrote:
>>>>>>>>> 26/05/2023 10:42, Chengwen Feng:
>>>>>>>>>> Add tracepoints at important APIs for tracing support.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>>>>>>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> v4: Fix asan smoke fail.
>>>>>>>>>> v3: Address Morten's comment:
>>>>>>>>>>     Move stats_get and vchan_status and to trace_fp.h.
>>>>>>>>>> v2: Address Morten's comment:
>>>>>>>>>>     Make stats_get as fast-path trace-points.
>>>>>>>>>>     Place fast-path trace-point functions behind in version.map.
>>>>>>>>>
>>>>>>>>> There are more things to fix.
>>>>>>>>> First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
>>>>>>>>
>>>>>>>> It was already included by rte_dmadev.h:
>>>>>>>> diff --git a/lib/dmadev/rte_dmadev.h b/lib/dmadev/rte_dmadev.h
>>>>>>>> index e61d71959e..e792b90ef8 100644
>>>>>>>> --- a/lib/dmadev/rte_dmadev.h
>>>>>>>> +++ b/lib/dmadev/rte_dmadev.h
>>>>>>>> @@ -796,6 +796,7 @@ struct rte_dma_sge {
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  #include "rte_dmadev_core.h"
>>>>>>>> +#include "rte_dmadev_trace_fp.h"
>>>>>>>>
>>>>>>>>
>>>>>>>>> Note: you could have caught this if testing the example app for DMA.
>>>>>>>>> Second, you must avoid structs and enum in this header file,
>>>>>>>>
>>>>>>>> Let me explain the #if #endif logic:
>>>>>>>>
>>>>>>>> For the function:
>>>>>>>> uint16_t
>>>>>>>> rte_dma_completed(int16_t dev_id, uint16_t vchan, const uint16_t nb_cpls,
>>>>>>>> 		  uint16_t *last_idx, bool *has_error)
>>>>>>>>
>>>>>>>> The common trace implementation:
>>>>>>>> RTE_TRACE_POINT_FP(
>>>>>>>> 	rte_dma_trace_completed,
>>>>>>>> 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>>>>>>>> 			     const uint16_t nb_cpls, uint16_t *last_idx,
>>>>>>>> 			     bool *has_error, uint16_t ret),
>>>>>>>> 	rte_trace_point_emit_i16(dev_id);
>>>>>>>> 	rte_trace_point_emit_u16(vchan);
>>>>>>>> 	rte_trace_point_emit_u16(nb_cpls);
>>>>>>>> 	rte_trace_point_emit_ptr(idx_val);
>>>>>>>> 	rte_trace_point_emit_ptr(has_error);
>>>>>>>> 	rte_trace_point_emit_u16(ret);
>>>>>>>> )
>>>>>>>>
>>>>>>>> But it has a problem: for pointer parameter (e.g. last_idx and has_error), only record
>>>>>>>> the pointer value (i.e. address value).
>>>>>>>>
>>>>>>>> I think the pointer value has no mean (in particular, many of there pointers are stack
>>>>>>>> variables), the value of the pointer point to is meaningful.
>>>>>>>>
>>>>>>>> So I add the pointer reference like below (as V3 did):
>>>>>>>> RTE_TRACE_POINT_FP(
>>>>>>>> 	rte_dma_trace_completed,
>>>>>>>> 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>>>>>>>> 			     const uint16_t nb_cpls, uint16_t *last_idx,
>>>>>>>> 			     bool *has_error, uint16_t ret),
>>>>>>>> 	int has_error_val = *has_error;            // pointer reference
>>>>>>>> 	int last_idx_val = *last_idx;              // pointer reference
>>>>>>>> 	rte_trace_point_emit_i16(dev_id);
>>>>>>>> 	rte_trace_point_emit_u16(vchan);
>>>>>>>> 	rte_trace_point_emit_u16(nb_cpls);
>>>>>>>> 	rte_trace_point_emit_int(last_idx_val);    // record the value of pointer
>>>>>>>> 	rte_trace_point_emit_int(has_error_val);   // record the value of pointer
>>>>>>>> 	rte_trace_point_emit_u16(ret);
>>>>>>>> )
>>>>>>>>
>>>>>>>> Unfortunately, the above lead to asan failed. because in:
>>>>>>>> RTE_TRACE_POINT_REGISTER(rte_dma_trace_completed,
>>>>>>>> 	lib.dmadev.completed)
>>>>>>>> it will invoke rte_dma_trace_completed() with the parameter is undefined.
>>>>>>>>
>>>>>>>>
>>>>>>>> To solve this problem, consider the rte_dmadev_trace_points.c will include rte_trace_point_register.h,
>>>>>>>> and the rte_trace_point_register.h will defined macro: _RTE_TRACE_POINT_REGISTER_H_.
>>>>>>>>
>>>>>>>> so we update trace points as (as V4 did):
>>>>>>>> RTE_TRACE_POINT_FP(
>>>>>>>> 	rte_dma_trace_completed,
>>>>>>>> 	RTE_TRACE_POINT_ARGS(int16_t dev_id, uint16_t vchan,
>>>>>>>> 			     const uint16_t nb_cpls, uint16_t *last_idx,
>>>>>>>> 			     bool *has_error, uint16_t ret),
>>>>>>>> #ifdef _RTE_TRACE_POINT_REGISTER_H_
>>>>>>>> 	uint16_t __last_idx = 0;
>>>>>>>> 	bool __has_error = false;
>>>>>>>> 	last_idx = &__last_idx;                  // make sure the pointer has meaningful value.
>>>>>>>> 	has_error = &__has_error;                // so that the next pointer reference will work well.
>>>>>>>> #endif /* _RTE_TRACE_POINT_REGISTER_H_ */
>>>>>>>> 	int has_error_val = *has_error;
>>>>>>>> 	int last_idx_val = *last_idx;
>>>>>>>> 	rte_trace_point_emit_i16(dev_id);
>>>>>>>> 	rte_trace_point_emit_u16(vchan);
>>>>>>>> 	rte_trace_point_emit_u16(nb_cpls);
>>>>>>>> 	rte_trace_point_emit_int(last_idx_val);
>>>>>>>> 	rte_trace_point_emit_int(has_error_val);
>>>>>>>> 	rte_trace_point_emit_u16(ret);
>>>>>>>> )
>>>>>>>>
>>>>>>>>> otherwise it cannot be included alone.
>>>>>>>>> Look at what is done in other *_trace_fp.h files.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Whether enable_trace_fp is true or false, the v4 work well.
>>>>>>>> Below is that run examples with enable_trace_fp=true.
>>>>>>>>
>>>>>>>> ./dpdk-test --file-prefix=feng123 --trace=lib.dmadev.* -l 10-11
>>>>>>>
>>>>>>> This is the test application, not the example.
>>>>>>> Please make sure examples/dma/ is compiling.
>>>>>>
>>>>>> Work well with examples/dma (compiled with enable_trace_fp=true).
>>>>>
>>>>> Can you try with enable_trace_fp=false (the default)?
>>>>
>>>> It works well too.
>>>>
>>>>>
>>>>>>> Also, the test chkincs must run fine.
>>>>>>
>>>>>> chkincs ?
>>>>>
>>>>> If this a word you don't know, you can try "git grep" to better understand.
>>>>> There is a Meson option "check_includes" to enable chkincs.
>>>>>
>>>>> I recommend using devtools/test-meson-builds.sh to test patches,
>>>>> it includes the above options.
>>>>
>>>> According your suggest, I use test-meson-builds.sh, and pass.
>>>
>>> It does not pass for me:
>>>
>>> In file included from dmafwd.c:14:
>>> build-x86-generic/install/usr/local/include/rte_dmadev.h:799:10:
>>> fatal error: rte_dmadev_trace_fp.h: No such file or directory
>>>   799 | #include "rte_dmadev_trace_fp.h"
>>
>> I still can't reproduce the above error with .develconfig contain
>> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all"".
>>
>> Could you provide the config options (which load by devtools/load-devel-config) ?
>>
>>>
>>> Let me repeat again my recommendations:
>>> First you must export rte_dmadev_trace_fp.h as it is included by rte_dmadev.h.
>>> 	YOU NEED TO ADD IT in meson.build FILE
>>> Note: you could have caught this if testing the example app for DMA.
>>> Second, you must avoid structs and enum in this header file,
>>
>> Yes, I found compile error with .develconfig contain
>> "DPDK_MESON_OPTIONS="max_numa_nodes=1 disable_drivers=event/cnxk examples=all enable_trace_fp=true""
>>
>> The root cause is that the structs (e.g. struct rte_dma_sge) and enum (e.g. enum rte_dma_status_code)
>> usage in dmadev fastpath API.
>>
>> I try to include "rte_dmadev.h" and it also not work (more compiler error).
>>
>> So I think two options:
>> 1. don't support fastpath tracepoints with dmadev library
>> 2. exclude xxx_trace_fp.h in buildtools/chkincs
>>
>> Would like to hear your opinion.
> 
> I've merged the patch for control path.
> Please send a new patch for data path, I will test it,
> and I will work with you to understand what happens.
> Let's target it for 24.03 release.

Got it, thanks a lot.

> 
> Thanks
> 
> 
> .
> 

  reply	other threads:[~2023-11-07  1:27 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-12  2:48 [PATCH] " Chengwen Feng
2023-04-12  9:52 ` Bruce Richardson
2023-04-13  3:44   ` fengchengwen
2023-04-13  8:45     ` Bruce Richardson
2023-04-12 11:00 ` Morten Brørup
2023-04-13  6:30   ` fengchengwen
2023-04-13  8:25     ` Morten Brørup
2023-04-15  0:33 ` [PATCH v3] " Chengwen Feng
2023-05-24 21:12   ` Thomas Monjalon
2023-05-27  0:17     ` fengchengwen
2023-05-26  8:42 ` [PATCH v4] " Chengwen Feng
2023-07-03  3:54   ` fengchengwen
2023-07-07 10:40   ` Thomas Monjalon
2023-07-09  3:23     ` fengchengwen
2023-07-10  6:49       ` Thomas Monjalon
2023-07-10  7:50         ` fengchengwen
2023-07-31 12:48           ` Thomas Monjalon
2023-08-03  7:52             ` fengchengwen
2023-08-14 14:16               ` Thomas Monjalon
2023-10-11  9:55                 ` fengchengwen
2023-11-06 20:59                   ` Thomas Monjalon
2023-11-07  1:26                     ` fengchengwen [this message]
2024-01-12 10:38                     ` fengchengwen
2023-10-20  2:21 ` [PATCH] dmadev: add tracepoints at control path APIs Chengwen Feng
2023-11-06 20:55   ` Thomas Monjalon

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=36c44f2f-fb5d-e23b-6289-c0a908aede43@huawei.com \
    --to=fengchengwen@huawei.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=kevin.laatz@intel.com \
    --cc=mb@smartsharesystems.com \
    --cc=thomas@monjalon.net \
    /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).