Date:   Thu, 2 Feb 2023 10:17:08 +0100
From:   Íñigo Huguet <ihuguet@...hat.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     Jakub Kicinski <kuba@...nel.org>, ecree.xilinx@...il.com,
        habetsm.xilinx@...il.com, richardcochran@...il.com,
        davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
        netdev@...r.kernel.org, Yalin Li <yalli@...hat.com>,
        kernel test robot <lkp@...el.com>
Subject: Re: [PATCH net-next v2 0/4] sfc: support unicast PTP

On Thu, Feb 2, 2023 at 9:38 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> On Thu, Feb 02, 2023 at 08:08:10AM +0100, Íñigo Huguet wrote:
> > On Wed, Feb 1, 2023 at 8:05 PM Jakub Kicinski <kuba@...nel.org> wrote:
> > >
> > > On Wed,  1 Feb 2023 09:08:45 +0100 Íñigo Huguet wrote:
> > > > v2: fixed missing IS_ERR
> > > >     added doc of missing fields in efx_ptp_rxfilter
> > >
> > > 1. don't repost within 24h, *especially* if you're reposting
> > > because of compilation problems
> > >
> > > https://www.kernel.org/doc/html/next/process/maintainer-netdev.html
> >
> > Sorry, I wasn't aware of this.
> >
> > > 2. please don't repost in a thread, it makes it harder for me
> > > to maintain a review queue
> >
> > What do you mean? I sent it with `git send-email --in-reply-to`, I
> > thought this was the canonical way to send a v2 and superseed the
> > previous version.
>
> It was never canonical way. I'm second to Jakub, it messes review and
> acceptance flow so badly that I prefer to do not take such patches due
> to possible confusion.

I was so sure about this that it has confused me a lot. But I've found
where my mistake came from: in the past I made a few contributions to
the Buildroot project, and there they explicitly request to do it
because they say that patchwork automatically marks the previous
version as superseded. But yes, of course you're right: in kernel's
documentation it explicitly says not to do it.

>
> >
> > > 3. drop the pointless inline in the source file in patch 4
> > >
> > > +static inline void efx_ptp_remove_one_filter(struct efx_nic *efx,
> > > +                                            struct efx_ptp_rxfilter *rxfilter)
> >
> > This is the second time I get pushback because of this. Could you
> > please explain the rationale of not allowing it?
> >
> > I understand that the compiler probably will do the right thing with
> > or without the `inline`, and more being in the same translation unit.
> > Actually, I checked the style guide [1] and I thought it was fine like
> > this: it says that `inline` should not be abused, but it's fine in
> > cases like this one. Quotes from the guide:
> >   "Generally, inline functions are preferable to macros resembling functions"
> >   "A reasonable rule of thumb is to not put inline at functions that
> > have more than 3 lines of code in them"
> >
> > I have the feeling that if I had made it as a macro it had been
> > accepted, but inline not, despite the "prefer inline over macro".
> >
> > I don't mind changing it, but I'd like to understand it so I can
> > remember the next time. And if it's such a hard rule that it's
> > considered a "fail" in the patchwork checks, maybe it should be
> > documented somewhere.
>
> GCC will automatically inline your not-inline function anyway.
>
> Documentation/process/coding-style.rst
>    958 15) The inline disease
> ...
>    978 Often people argue that adding inline to functions that are static and used
>    979 only once is always a win since there is no space tradeoff. While this is
>    980 technically correct, gcc is capable of inlining these automatically without
>    981 help, and the maintenance issue of removing the inline when a second user
>    982 appears outweighs the potential value of the hint that tells gcc to do
>    983 something it would have done anyway.

I also saw that, but this paragraph seems to talk about functions of
*any* size, for which many people think that it's good to add `inline`
if they're used *only once*. That's not this case, but instead this
case seems to fit well in the cases where the guide says that it's OK
to use them:
"Generally, inline functions are preferable to macros resembling functions"
"A reasonable rule of thumb is to not put inline at functions that
have more than 3 lines of code in them".

Just to be clear: there are a lot of discussions and opinions about
how to use inline, and some rules about its usage are needed (mainly
limiting it). What I mean is that we have some written rules, but if
there are additional rules that are being applied, maybe they should
be written too. That way we would avoid work in reviews and resends
(because I checked the coding-style regarding this topic before
sending the patch), and we the developers would understand better the
technical reasons behind it.

> > Thanks!
> >
> > [1] https://www.kernel.org/doc/html/latest/process/coding-style.html
> >
> >
> > --
> > Íñigo Huguet
> >
>


-- 
Íñigo Huguet

Powered by blists - more mailing lists