| Message ID | 20230201080849.10482-1-ihuguet@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | sfc: support unicast PTP | expand |
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 2. please don't repost in a thread, it makes it harder for me to maintain a review queue 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)
On Wed, Feb 1, 2023 at 8:05 PM Jakub Kicinski <kuba@kernel.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. > 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. Thanks! [1] https://www.kernel.org/doc/html/latest/process/coding-style.html
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@kernel.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. > > > 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. > > Thanks! > > [1] https://www.kernel.org/doc/html/latest/process/coding-style.html > > > -- > Íñigo Huguet >
On Thu, Feb 2, 2023 at 9:38 AM Leon Romanovsky <leon@kernel.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@kernel.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 > > >
Unicast PTP was not working with sfc NICs. The reason was that these NICs don't timestamp all incoming packets, but instead they only timestamp packets of the queues that are selected for that. Currently, only one RX queue is configured for timestamp: the RX queue of the PTP channel. The packets that are put in the PTP RX queue are selected according to firmware filters configured from the driver. Multicast PTP was already working because the needed filters are known in advance, so they're inserted when PTP is enabled. This patches add the ability to dynamically add filters for unicast addresses, extracted from the TX PTP-event packets. Since we don't know in advance how many filters we'll need, some info about the filters need to be saved. This will allow to check if a filter already exists or if a filter is too old and should be removed. Note that the previous point is unnecessary for multicast filters, but I've opted to change how they're handled to match the new unicast's filters to avoid having duplicate insert/remove_filters functions, once for each type of filter. Tested: With ptp4l, all combinations of master/slave and unicast/multicast Reported-by: Yalin Li <yalli@redhat.com> Signed-off-by: Íñigo Huguet <ihuguet@redhat.com> v2: fixed missing IS_ERR added doc of missing fields in efx_ptp_rxfilter For the missing IS_ERR: Reported-by: kernel test robot <lkp@intel.com> Íñigo Huguet (4): sfc: store PTP filters in a list sfc: allow insertion of filters for unicast PTP sfc: support unicast PTP sfc: remove expired unicast PTP filters drivers/net/ethernet/sfc/ptp.c | 274 ++++++++++++++++++++++++++------- 1 file changed, 219 insertions(+), 55 deletions(-) -- 2.34.3