Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e002b414-0e12-0ee8-08a9-2a2b2f21c7bc@oracle.com>
Date:   Wed, 19 Jul 2023 10:02:06 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v2 0/9] tracing: Improbe BTF support on probe events
Content-Language: en-GB
To:     "Masami Hiramatsu (Google)" <mhiramat@kernel.org>,
        linux-trace-kernel@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>,
        Martin KaFai Lau <martin.lau@linux.dev>, bpf@vger.kernel.org,
        Sven Schnelle <svens@linux.ibm.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Jiri Olsa <jolsa@kernel.org>
References: <168960739768.34107.15145201749042174448.stgit@devnote2>
From:   Alan Maguire <alan.maguire@oracle.com>
In-Reply-To: <168960739768.34107.15145201749042174448.stgit@devnote2>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TGpaR3NNQXM4SDRhcWxnVnAxTzFFeHFZWEhCZmNrVmR5Y2QxdFY5SDZ1YzF1?=
 =?utf-8?B?bkZGSFdzcnphNUl0UUVLb0I2UG1qRHlmR25nV1JZdlI2cnZ4SDlLZWJyREw5?=
 =?utf-8?B?Y1FIMFp6VmgxdlBKN3dIOFpDakF0V0FVN2cvZGlEVTYyUlRpWW0yQnZCNXlv?=
 =?utf-8?B?eVJOSjlETW9NaUhkZ3FEOEhLUFFRUFVZL0k3NDl1NEtPbm9oSWJlYlk3Y0Vh?=
 =?utf-8?B?UXRaazVVTGpVQ2pSdWNrWk52MmNtV08vbEV4N2lwaExiSHpod2FIM3BHQTFE?=
 =?utf-8?B?UzlrTGY0ejRONVpqR1JNZVZBRVIyY3VCaHAwTjdWWklEVjBZQUdKSWo2ell3?=
 =?utf-8?B?NS9aOWd5VisrL1Z2NTBGNjNrM2ViK0dVdVh5VU5Wa3BORnVwYXgrOWZ0NDBK?=
 =?utf-8?B?L3NsRmQ2TGc4czBPSHB5aWN1eXk3Tk9pTmNWRW0zVTk4MUhBVWRPbnhHdk45?=
 =?utf-8?B?MGdmZUs5UU45alpwdGk3akRnb0tMMGJzMzZXSnZZUUE5RnNJWFRLUk16Zysv?=
 =?utf-8?B?NHNiR2Q2a2NROHNrRVlWZFk0M1J4TDMweE1vMXR3azd4bW81L3Rybjh5Z2FX?=
 =?utf-8?B?a2gxQWV1K0RoUWNNczZKRXgvRUlCRzJRL2hZTXRZM0hEaHoyS1NmRDFtc0lW?=
 =?utf-8?B?TW55blU5NU94M0ZBcE1VbE9DUWNCWXBuV0UrakhoRGlWL29XSjRjY3JOQnM3?=
 =?utf-8?B?azNXYkE4TkJtcGRVQmFFN1BGUzdzQ0ErQ1lFOTBSWXVaa1cvcDRpN0RCOENi?=
 =?utf-8?B?VHc4U1lUL0c4TExMUWlwY0pIdDlSWXhRcjZGMWhZVU1GNkNOcmU3MWl5cGEw?=
 =?utf-8?B?eGhYNDZGblN6R2U3YlFRSGt1bzlvZzllMzFMLzk4b0Z1ZGcxbFVmOGVxT0g0?=
 =?utf-8?B?ZUZrTnU3bDlmclhrQjM3cmtRRWxSQWJkTFhOcTExWHc4cWRVSUJ5OThCTTha?=
 =?utf-8?B?UEhqOUZvUzBBc1BhdjFWUE44amVoaE5mbkNjOFRLbDJQYTlpK0pBQ3pKSHY1?=
 =?utf-8?B?a0VXM3ZMQlNJd25BQ2hjWVFpTmZsaEZ3Q3YvaU1abVJUejY0WUw3ckRDQzVC?=
 =?utf-8?B?VFZWSUw2YTVVaTRVVW5tc1ptOHFYM3VzL1d0ZWswWXBlZ3JwL21iY3JFSk1K?=
 =?utf-8?B?OEJIeWJCT0JQZzZlTHUrVFFYeDJlazNJejdJUXdmd2tQV2dYNjJFUEN6OG5y?=
 =?utf-8?B?SGRmSi9lOFo3VEpraElTRzlvVWFVZ2I1T1R0ZWxJL2RvRVFDSmRXTDBHMzVU?=
 =?utf-8?B?R2xuVmI0N3V5M0g1a2xFVVB3OUpsWUQva2QzN2ZPWG1UaUlOOTE5RkNtWGtG?=
 =?utf-8?B?RE12YmpVV1pYT3JCWjZDeWw2VmhjS244a1BabnlxaDB0NXJERnRoV3QyNGNQ?=
 =?utf-8?B?TEFyZUNleDFMRDZUemtzZkNyUXRHTXUxM1d0ZFBsNktmeENWRWkvc2t3Q1kx?=
 =?utf-8?B?Mzl2cnpLSlZsNElsZ2hYeE00TkhGU2RZREI3VWVjYmhBMFRKanRZNE11ZWZ6?=
 =?utf-8?B?VWxlM2o3dEdsTEp3Z2c4Vm83MGI3ZE55WWsrNW1TNTJ2SlRuck1vTFpvb1g0?=
 =?utf-8?B?WnpSTmFaajF2VkxFTFRYcjA5MEJtS3pLNjlPZnZMOVVDRmRZdWJPbEFNcU1B?=
 =?utf-8?B?RzBVV3RFWGhyNERoeGkrbGxvVjc3a3Z1L3Q5MDJON3VXVGtURzJHTTRoaENR?=
 =?utf-8?B?QVBEUVQrNnFab3hYUUVxS2xTT1hFMHFIc052b0x5c0pnUWJpeW9OcGxMemRv?=
 =?utf-8?B?cnlpL0dIMThyWUJTR3ZLZEtHOUNBQitEQ2p5WW0yVlh6S1U4SXp3TUdzcXM0?=
 =?utf-8?B?ZXZCeHJ6bDRpV3NDZzRabm1TNmltUEFtanY3alVZdHRjMGROKzExSHBnMzVz?=
 =?utf-8?B?cmh0TnF0Kzk0UXpGcE9JREc0TnE0SzBSakhTdnY2KytRcnVuUlQ4ZmJWTTU4?=
 =?utf-8?B?dmdwNWMrQ2RqeTlZU3FmeDlNRmRCQ2YyZDgwWVpBdTNJZHhkOWtYMG5WQlY1?=
 =?utf-8?B?b2duSEdBMTlNZGNJY21wMHJzSWw4NWNxTEVsSXlnSTVYMmRrdkt4Zk1PSFNq?=
 =?utf-8?B?cXJhYnJ0K1JJakdpcC82YU9KWmdXUHl5M3JwK215RXZEazVuYTdCVkZKaENB?=
 =?utf-8?B?RGh3aDhoa1NTNnRlTURPSzFzMy93L3N1c0E2SWRhKzhiNXNjNEZGaUJpY0xZ?=
 =?utf-8?Q?C8T1mLExbT6ZiQvzuR2dr/g=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 4h1lbUV7aoxP8VLwpM40MSDpQImFNdIcZUBs9ZK20hjUjT5Hs0a5yw1FfN/NTx8Slb1pUUSUmKOjiqgP9MkrgZZ9rKhG8NdaGKEcbH7NXZPfvXzBPYaUJrESwJogJtD/6XTX+YKgsRRdSfpkzl2bU1zjiHvCFoCCesBhbt8w/zWIkPCBi+PnLOpprv9eu2byuyT8BGxPEEdnDBdGDe60cl8M4wbNGUf6dARPfHHrZgRy1wKT91JCdH3hOR1v80WEu4CwiCT6iuFIm7ERMaM6Py2VDofFgnHezcRqXRWE39gURSOaiC91tp+agyvtZcH+YYY0FAU0m7dmiCH0A8TqegFW0j27ELWUEJ3NJ9c/5XDry0MN7SIgAht2yTZtEN7aEKw45HdSOLwG/8EcMwY+aYOyiyiuu+dXTn0JKWXjr2trdMO8u+Q54qD0vKfirtnBfioX5YSZG4YDPfKHAIZt71U22aokEiLBS1f06n7ubX+ZlBg7Pb5sdkf61uP3a680ze8BDl9YVDs0kvBeJBp01uS9Cou7bvauLQWyRlKcAgTZmFHgfSUrHWJ0YphTPpRiV0fwMdWN4jfYpXTurx/OJg/EV4ucGzzOWHyL8qxVnXCkt5UEUOZHaCK8myCfPNMN6EuqAZ5uw0lwlcF50iNXT+xRMAFA2yc8sKg62WsARRnKKObS7Rltz4VqhE1h1wtuHMAPctTl3op6/wUAyWWq5YIMD2+JOrE54rNZIphOkiuJxg81L/Jl6ump4GUkuFKe5zN5f3u4lQMdV9Z913a1nqMEzAOMdyZoNwaWWp2uSQGDUQpVYfLgO4IvSs387E18EYQMbkpeuXnfjSVL5nbq38lYRMF+z5vfPSeAr79xZmyZhUeqmjzkpInbgUII1oTr
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4b3f8816-a9fa-43a7-5c73-08db8836d69a
X-MS-Exchange-CrossTenant-AuthSource: BLAPR10MB5267.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jul 2023 09:02:11.4995
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: R58+0v3oGJRCxq+Z9K2r/QPOcLDb9b1SJb0ZOcF0oh1hu3/8hpY2fal+lfV04jf9wFIQZy7SQBPcyPP2DTzq1A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5648
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-07-19_05,2023-07-18_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 mlxscore=0 adultscore=0 spamscore=0 phishscore=0 bulkscore=0
 mlxlogscore=999 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2306200000 definitions=main-2307190082
X-Proofpoint-ORIG-GUID: UUipyJw9d919Rp3F14AXo28CSEO5Qg7G
X-Proofpoint-GUID: UUipyJw9d919Rp3F14AXo28CSEO5Qg7G
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 17/07/2023 16:23, Masami Hiramatsu (Google) wrote:
> Hi,
> 
> Here is the 2nd version of series to improve the BTF support on probe events.
> The previous series is here:
> 
> https://lore.kernel.org/linux-trace-kernel/168699521817.528797.13179901018528120324.stgit@mhiramat.roam.corp.google.com/
> 
> In this version, I added a NULL check fix patch [1/9] (which will go to
> fixes branch) and move BTF related API to kernel/bpf/btf.c [2/9] and add
> a new BTF API [3/9] so that anyone can reuse it.
> Also I decided to use '$retval' directly instead of 'retval' pseudo BTF
> variable for field access at [5/9] because I introduced an idea to choose
> function 'exit' event automatically if '$retval' is used [7/9]. With that
> change, we can not use 'retval' because if a function has 'retval'
> argument, we can not decide 'f func retval' is function exit or entry.

this is fantastic work! (FWIW I ran into the retval argument issue with
ksnoop as well; I got around it by using "return" to signify the return
value since as a reserved word it won't clash with a variable name.
However in the trace subsystem context retval is used extensively so
makes sense to stick with that).

One thing we should probably figure out is a common approach to handling
ambiguous static functions that will work across ftrace and BPF.  A few
edge cases that are worth figuring out:

1. a static function with the same name exists in multiple modules,
either with different or identical function signatures
2. a static function has .isra.0 and other gcc suffixes applied to
static functions during optimization

As Alexei mentioned, we're still working on 1, so it would be good
to figure out a naming scheme that works well in both ftrace and BPF
contexts. There are a few hundred of these ambiguous functions. My
reading of the fprobe docs seems to suggest that there is no mechanism
to specify a specific module for a given symbol (as in ftrace filters),
is that right?

Jiri led a session on this topic at LSF/MM/BPF ; perhaps we should
carve out some time at Plumbers to discuss this?

With respect to 2, pahole v1.25 will generate representations for these
"."-suffixed functions in BTF via --btf_gen_optimized [1]. (BTF
representation is skipped if the optimizations impact on the registers
used for function arguments; if these don't match calling conventions
due to optimized-out params, we don't represent the function in BTF,
as the tracing expectations are violated).

However the BTF function name - in line with DWARF representation -
will not have the .isra suffix. So the thing to bear in mind is if
you use the function name with suffix as the fprobe function name,
a BTF lookup of that exact ("foo.isra.0") name will not find anything,
while a lookup of "foo" will succeed. I'll add some specifics in your
patch doing the lookups, but just wanted to highlight the issue at
the top-level.

Thanks!

Alan

[1]
https://lore.kernel.org/bpf/1675790102-23037-1-git-send-email-alan.maguire@oracle.com/

> Selftest test case [8/9] and document [9/9] are also updated according to
> those changes.
> 
> This series can be applied on top of "v6.5-rc2" kernel.
> 
> You can also get this series from:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mhiramat/linux.git topic/fprobe-event-ext
> 
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (Google) (9):
>       tracing/probes: Fix to add NULL check for BTF APIs
>       bpf/btf: tracing: Move finding func-proto API and getting func-param API to BTF
>       bpf/btf: Add a function to search a member of a struct/union
>       tracing/probes: Support BTF based data structure field access
>       tracing/probes: Support BTF field access from $retval
>       tracing/probes: Add string type check with BTF
>       tracing/fprobe-event: Assume fprobe is a return event by $retval
>       selftests/ftrace: Add BTF fields access testcases
>       Documentation: tracing: Update fprobe event example with BTF field
> 
> 
>  Documentation/trace/fprobetrace.rst                |   50 ++
>  include/linux/btf.h                                |    7 
>  kernel/bpf/btf.c                                   |   83 ++++
>  kernel/trace/trace_fprobe.c                        |   58 ++-
>  kernel/trace/trace_probe.c                         |  402 +++++++++++++++-----
>  kernel/trace/trace_probe.h                         |   12 +
>  .../ftrace/test.d/dynevent/add_remove_btfarg.tc    |   11 +
>  .../ftrace/test.d/dynevent/fprobe_syntax_errors.tc |    6 
>  8 files changed, 503 insertions(+), 126 deletions(-)
> 
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
>