Date:   Fri, 3 Feb 2023 08:56:33 -0600
From:   David Vernet <void@...ifault.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
        yhs@...a.com, john.fastabend@...il.com, kpsingh@...nel.org,
        sdf@...gle.com, haoluo@...gle.com, jolsa@...nel.org,
        linux-kernel@...r.kernel.org, kernel-team@...a.com,
        brouer@...hat.com, corbet@....net, linux-doc@...r.kernel.org
Subject: Re: [PATCH bpf-next v2] bpf/docs: Document kfunc lifecycle /
 stability expectations

On Fri, Feb 03, 2023 at 08:47:21AM -0600, David Vernet wrote:
> On Fri, Feb 03, 2023 at 02:04:10PM +0100, Toke Høiland-Jørgensen wrote:
> > David Vernet <void@...ifault.com> writes:
> > 
> > > BPF kernel <-> kernel API stability has been discussed at length over
> > > the last several weeks and months. Now that we've largely aligned over
> > > kfuncs being the way forward, and BPF helpers being considered frozen,
> > > it's time to document the expectations for kfunc lifecycles and
> > > stability so that everyone (BPF users, kfunc developers, and
> > > maintainers) are all aligned, and have a crystal-clear understanding of
> > > the expectations surrounding kfuncs.
> > >
> > > To do that, this patch adds that documentation to the main kfuncs
> > > documentation page via a new 'kfunc lifecycle expectations' section. The
> > > patch describes how decisions are made in the kernel regarding whether
> > > to include, keep, deprecate, or change / remove a kfunc. As described
> > > very overtly in the patch itself, but likely worth highlighting here:
> > >
> > > "kfunc stability" does not mean, nor ever will mean, "BPF APIs may block
> > > development elsewhere in the kernel".
> > >
> > > Rather, the intention and expectation is for kfuncs to be treated like
> > > EXPORT_SYMBOL_GPL symbols in the kernel. The goal is for kfuncs to be a
> > > safe and valuable option for maintainers and kfunc developers to extend
> > > the kernel, without tying anyone's hands, or imposing any kind of
> > > restrictions on maintainers in the same way that UAPI changes do.
> > >
> > > In addition to the 'kfunc lifecycle expectations' section, this patch
> > > also adds documentation for a new KF_DEPRECATED kfunc flag which kfunc
> > > authors or maintainers can choose to add to kfuncs if and when they
> > > decide to deprecate them. Note that as described in the patch itself, a
> > > kfunc need not be deprecated before being changed or removed -- this
> > > flag is simply provided as an available deprecation mechanism for those
> > > that want to provide a deprecation story / timeline to their users.
> > > When necessary, kfuncs may be changed or removed to accommodate changes
> > > elsewhere in the kernel without any deprecation at all.
> > >
> > > Signed-off-by: David Vernet <void@...ifault.com>
> > 
> > Some comments below, but otherwise please add my:
> > 
> > Co-developed-by: Toke Høiland-Jørgensen <toke@...hat.com>
> > 
> > should we Cc the next version to linux-api@...r as well just to get a
> > bit more visibility in case others have comments?
> 
> Yes, good idea, will do.
> 
> > 
> > > ---
> > >  Documentation/bpf/kfuncs.rst | 138 +++++++++++++++++++++++++++++++++--
> > >  1 file changed, 133 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > > index 0bd07b39c2a4..4135f3111b67 100644
> > > --- a/Documentation/bpf/kfuncs.rst
> > > +++ b/Documentation/bpf/kfuncs.rst
> > > @@ -13,7 +13,7 @@ BPF Kernel Functions or more commonly known as kfuncs are functions in the Linux
> > >  kernel which are exposed for use by BPF programs. Unlike normal BPF helpers,
> > >  kfuncs do not have a stable interface and can change from one kernel release to
> > >  another. Hence, BPF programs need to be updated in response to changes in the
> > > -kernel.
> > > +kernel. See :ref:`BPF_kfunc_lifecycle_expectations` for more information.
> > >  
> > >  2. Defining a kfunc
> > >  ===================
> > > @@ -238,6 +238,32 @@ single argument which must be a trusted argument or a MEM_RCU pointer.
> > >  The argument may have reference count of 0 and the kfunc must take this
> > >  into consideration.
> > >  
> > > +.. _KF_deprecated_flag:
> > > +
> > > +2.4.9 KF_DEPRECATED flag
> > > +------------------------
> > > +
> > > +The KF_DEPRECATED flag is used for kfuncs which are expected to be changed or
> > > +removed in a subsequent kernel release. Deprecated kfuncs may be removed at any
> > > +time, though if possible (and when applicable), developers are encouraged to
> > > +provide users with a deprecation window to ease the burden of migrating off of
> > > +the kfunc.
> > 
> > 
> > 
> > I think the "may be removed at any time" is a bit odd here. If someone
> > wants to just remove a kfunc, why bother with the deprecation flag at
> > all? Besides, that whole "deprecation is optional" bit is explained
> 
> I definitely agree that the phrasing could be improved, but my intention
> with that phrase was actually to answer the exact nuanced question you
> posed. I think we need to clarify that KF_DEPRECATED is an optional tool
> that developers can use to provide a softer deprecation story to their
> users, rather than a flag that exists as part of a larger, strict
> deprecation policy. Otherwise, I think it would be unclear to someone
> reading this when and why they would need to use the flag. I liked your
> suggestion below, and proposed a bit of extra text to try and capture
> this point. Lmk what you think.
> 
> > below, in this section we're just explaining the flag. So I'd just drop
> > this bit and combine the first two paragraphs as:
> > 
> > "The KF_DEPRECATED flag is used for kfuncs which are scheduled to be
> > changed or removed in a subsequent kernel release. A kfunc that is
> > marked with KF_DEPRECATED should also have any relevant information
> > captured in its kernel doc. Such information typically includes the
> > kfunc's expected remaining lifespan, a recommendation for new
> > functionality that can replace it if any is available, and possibly a
> > rationale for why it is being removed."
> 
> I like this -- are you OK with adding this in a small subsequent
> paragraph to address the point I made above?
> 
> Note that KF_DEPRECATED is simply a tool that developers can choose to
> use to ease their users' burden of migrating off of a kfunc. While
> developers are encouraged to use KF_DEPRECATED to provide a
> transitionary deprecation period to users of their kfunc, doing so is
> not strictly required, and the flag does not exist as part of any larger
> kfunc deprecation policy.

Nevermind, after reading this a few more times, I think what you
proposed above is sufficient. Will not be including this extra paragraph
in v3.

[...]

Powered by blists - more mailing lists