LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Song Liu <songliubraving@fb.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "open list:BPF (Safe dynamic programs and tools)" 
	<bpf@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ingo Molnar <mingo@redhat.com>, kajoljain <kjain@linux.ibm.com>,
	Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 1/3] perf: enable branch record for software events
Date: Mon, 30 Aug 2021 16:36:44 +0000	[thread overview]
Message-ID: <106E85E3-D30C-48BC-8EB6-8DFEEECF2022@fb.com> (raw)
In-Reply-To: <YS0CBphTuIdTWEXF@hirez.programming.kicks-ass.net>



> On Aug 30, 2021, at 9:06 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> 
> On Mon, Aug 30, 2021 at 03:25:44PM +0000, Song Liu wrote:
>> Thanks for these information! I did get confused these macros for quite a 
>> while. Let me try with the _RET0 version.
> 
> Does you kernel have:
> 
>  9ae6ab27f44e ("static_call: Update API documentation")
> 
> ?
> 
> With that included, the comment at the top of static_call.h reads like
> the below. Please let me know where you think this can be improved.

Aha, my kernel has the code for _RET0 part, but not the documentation. 

> 
> /*
> * Static call support
> *
> * Static calls use code patching to hard-code function pointers into direct
> * branch instructions. They give the flexibility of function pointers, but
> * with improved performance. This is especially important for cases where
> * retpolines would otherwise be used, as retpolines can significantly impact
> * performance.
> *

[...]

> *
> * Notes on NULL function pointers:
> *
> *   Static_call()s support NULL functions, with many of the caveats that
> *   regular function pointers have.
> *
> *   Clearly calling a NULL function pointer is 'BAD', so too for
> *   static_call()s (although when HAVE_STATIC_CALL it might not be immediately
> *   fatal). A NULL static_call can be the result of:
> *

Probably add:

 *     /* for function that returns NULL */
> *     DECLARE_STATIC_CALL_NULL(my_static_call, void (*)(int));


 *   or 
 *     /* for function that returns int */
 *     DECLARE_STATIC_CALL_RET0(my_static_call, int (*)(int));
 * 

So it is clear that we need two different macros. IIUC, the number and 
type of arguments doesn't matter? 

Also, the default return int function has to return 0, right? Can we let 
it return -EOPNOSUPP? 


> *
> *   which is equivalent to declaring a NULL function pointer with just a
> *   typename:
> *
> *     void (*my_func_ptr)(int arg1) = NULL;
> *

[...]

> *   which will include the required value tests to avoid NULL-pointer
> *   dereferences.
> *


> *   To query which function is currently set to be called, use:
> *
> *   func = static_call_query(name);

Maybe move above two lines to "Usage example:" section? 

Thanks,
Song

  reply	other threads:[~2021-08-30 16:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-26 22:13 [PATCH v2 bpf-next 0/3] bpf: introduce bpf_get_branch_snapshot Song Liu
2021-08-26 22:13 ` [PATCH v2 bpf-next 1/3] perf: enable branch record for software events Song Liu
2021-08-30 10:22   ` Peter Zijlstra
2021-08-30 15:25     ` Song Liu
2021-08-30 16:06       ` Peter Zijlstra
2021-08-30 16:36         ` Song Liu [this message]
2021-09-01 17:09           ` Peter Zijlstra
2021-08-30 17:41     ` Song Liu
2021-08-30 18:07       ` Peter Zijlstra
2021-09-01 17:12         ` Peter Zijlstra
2021-09-04 23:01           ` Josh Poimboeuf
2021-08-30 10:43   ` Peter Zijlstra
2021-08-30 16:06     ` Song Liu
2021-08-26 22:13 ` [PATCH v2 bpf-next 2/3] bpf: introduce helper bpf_get_branch_snapshot Song Liu
2021-08-27  9:28   ` kernel test robot
2021-08-27 15:10   ` kernel test robot
2021-08-30 10:47     ` Peter Zijlstra
2021-08-26 22:13 ` [PATCH v2 bpf-next 3/3] selftests/bpf: add test for bpf_get_branch_snapshot Song Liu

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=106E85E3-D30C-48BC-8EB6-8DFEEECF2022@fb.com \
    --to=songliubraving@fb.com \
    --cc=Kernel-team@fb.com \
    --cc=acme@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=kjain@linux.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --subject='Re: [PATCH v2 bpf-next 1/3] perf: enable branch record for software events' \
    /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

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).