Messages in this thread | | | From | Willem de Bruijn <> | Date | Fri, 30 Oct 2020 17:11:24 -0400 | Subject | Re: [PATCH net-next v4 1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames |
| |
On Fri, Oct 30, 2020 at 4:02 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Fri, Oct 30, 2020 at 9:35 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > In general we try to avoid changing counter behavior like that, as > > existing users > > may depend on current behavior, e.g., in dashboards or automated monitoring. > > > > I don't know how realistic that is in this specific case, no strong > > objections. Use > > good judgment. > > Originally this function only increases stats.rx_dropped only when > there's a memory squeeze. I don't know the specification for the > meaning of stats.rx_dropped, but as I understand it indicates a frame > is dropped. This is why I wanted to increase it whenever we drop a > frame.
Jakub recently made stats behavior less ambiguous, in commit 0db0c34cfbc9 ("net: tighten the definition of interface statistics").
That said, it's not entirely clear whether rx_dropped would be allowed to include rx_errors.
My hunch is that it shouldn't. A quick scan of devices did quickly show at least one example where it does: macvlan. But I expect that to be an outlier.
> Originally this function drops a frame silently if the PVC virtual > device that corresponds to the DLCI number and the protocol type > doesn't exist. I think we may at least need some way to note this. > Originally this function drops a frame with a kernel info message > printed if the protocol type is not supported. I think this is a bad > way because if the other end continuously sends us a lot of frames > with unsupported protocol types, our kernel message log will be > overwhelmed. > > I don't know how important it is to keep backwards compatibility. I > usually don't consider this too much. But I can drop this change if we > really want to keep the counter behavior unchanged. I think changing > it is better if we don't consider backwards compatibility.
Please do always consider backward compatibility. In this case, I don't think that the behavioral change is needed for the core of the patch (changing control flow).
| |