diff mbox series

[net-next,v4,1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames

Message ID 20201030022839.438135-2-xie.he.0141@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: hdlc_fr: Improve fr_rx and add support for any Ethertype | expand

Commit Message

Xie He Oct. 30, 2020, 2:28 a.m. UTC
1.
When the fr_rx function drops a received frame (because the protocol type
is not supported, or because the PVC virtual device that corresponds to
the DLCI number and the protocol type doesn't exist), the function frees
the skb and returns.

The code for freeing the skb and returning is repeated several times, this
patch uses "goto rx_drop" to replace them so that the code looks cleaner.

2.
Add code to increase the stats.rx_dropped count whenever we drop a frame.
Increase the stats.rx_dropped count both after "goto rx_drop" and after
"goto rx_error" because I think we should increase this value whenever an
skb is dropped.

Cc: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Krzysztof Halasa <khc@pm.waw.pl>
Signed-off-by: Xie He <xie.he.0141@gmail.com>
---
 drivers/net/wan/hdlc_fr.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Comments

Willem de Bruijn Oct. 30, 2020, 4:34 p.m. UTC | #1
On Thu, Oct 29, 2020 at 10:31 PM Xie He <xie.he.0141@gmail.com> wrote:
>
> 1.
> When the fr_rx function drops a received frame (because the protocol type
> is not supported, or because the PVC virtual device that corresponds to
> the DLCI number and the protocol type doesn't exist), the function frees
> the skb and returns.
>
> The code for freeing the skb and returning is repeated several times, this
> patch uses "goto rx_drop" to replace them so that the code looks cleaner.
>
> 2.
> Add code to increase the stats.rx_dropped count whenever we drop a frame.
> Increase the stats.rx_dropped count both after "goto rx_drop" and after
> "goto rx_error" because I think we should increase this value whenever an
> skb is dropped.

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.
Xie He Oct. 30, 2020, 8:01 p.m. UTC | #2
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.

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.
Willem de Bruijn Oct. 30, 2020, 9:11 p.m. UTC | #3
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).
Xie He Oct. 30, 2020, 9:27 p.m. UTC | #4
On Fri, Oct 30, 2020 at 2:12 PM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> 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.

OK. Thanks for the information.

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

OK. I'll drop the change about stats.rx_dropped. Thanks.
diff mbox series

Patch

diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c
index 409e5a7ad8e2..c774eff44534 100644
--- a/drivers/net/wan/hdlc_fr.c
+++ b/drivers/net/wan/hdlc_fr.c
@@ -904,8 +904,7 @@  static int fr_rx(struct sk_buff *skb)
 		netdev_info(frad, "No PVC for received frame's DLCI %d\n",
 			    dlci);
 #endif
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
 	if (pvc->state.fecn != fh->fecn) {
@@ -963,14 +962,12 @@  static int fr_rx(struct sk_buff *skb)
 		default:
 			netdev_info(frad, "Unsupported protocol, OUI=%x PID=%x\n",
 				    oui, pid);
-			dev_kfree_skb_any(skb);
-			return NET_RX_DROP;
+			goto rx_drop;
 		}
 	} else {
 		netdev_info(frad, "Unsupported protocol, NLPID=%x length=%i\n",
 			    data[3], skb->len);
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
 	if (dev) {
@@ -982,12 +979,13 @@  static int fr_rx(struct sk_buff *skb)
 		netif_rx(skb);
 		return NET_RX_SUCCESS;
 	} else {
-		dev_kfree_skb_any(skb);
-		return NET_RX_DROP;
+		goto rx_drop;
 	}
 
- rx_error:
+rx_error:
 	frad->stats.rx_errors++; /* Mark error */
+rx_drop:
+	frad->stats.rx_dropped++;
 	dev_kfree_skb_any(skb);
 	return NET_RX_DROP;
 }