| Message ID | 20201031004918.463475-2-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v6,1/5] net: hdlc_fr: Simpify fr_rx by using "goto rx_drop" to drop frames | expand |
On Fri, Oct 30, 2020 at 8:50 PM Xie He <xie.he.0141@gmail.com> wrote: > > 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. > > 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 | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > index 409e5a7ad8e2..4db0e01b96a9 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,12 @@ 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: > dev_kfree_skb_any(skb); > return NET_RX_DROP; I meant that I don't think errors should be double counted in rx_error and rx_drop. It is fine to count drops as either. Especially without that, I'm not sure this and the follow-on patch add much value. Minor code cleanups complicate backports of fixes.
On Sat, Oct 31, 2020 at 7:33 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > > - rx_error: > > +rx_error: > > frad->stats.rx_errors++; /* Mark error */ > > +rx_drop: > > dev_kfree_skb_any(skb); > > return NET_RX_DROP; > > I meant that I don't think errors should be double counted in rx_error > and rx_drop. It is fine to count drops as either. OK. Can we do that in another patch? Because I feel this would make the code a little bit more complex. Let's keep this patch as only a simple clean-up. > Especially without that, I'm not sure this and the follow-on patch add > much value. Minor code cleanups complicate backports of fixes. To me this is necessary, because I feel hard to do any development on un-cleaned-up code. I really don't know how to add my code without these clean-ups, and even if I managed to do that, I would not be happy with my code.
On Sat, Oct 31, 2020 at 8:18 AM Xie He <xie.he.0141@gmail.com> wrote: > > > Especially without that, I'm not sure this and the follow-on patch add > > much value. Minor code cleanups complicate backports of fixes. > > To me this is necessary, because I feel hard to do any development on > un-cleaned-up code. I really don't know how to add my code without > these clean-ups, and even if I managed to do that, I would not be > happy with my code. And always keeping the user interface and even the code unchanged contradicts my motivation of contributing to the Linux kernel. All my contributions are motivated by the hope to clean things up. I'm not an actual user of any of the code I contribute. If we adhere to the philosophy of not doing any clean-ups, my contributions would be meaningless.
On Sat, Oct 31, 2020 at 12:02 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Sat, Oct 31, 2020 at 8:18 AM Xie He <xie.he.0141@gmail.com> wrote: > > > > > Especially without that, I'm not sure this and the follow-on patch add > > > much value. Minor code cleanups complicate backports of fixes. > > > > To me this is necessary, because I feel hard to do any development on > > un-cleaned-up code. I really don't know how to add my code without > > these clean-ups, and even if I managed to do that, I would not be > > happy with my code. That is the reality of working in this space, I think. I have frequently restructured code, fixed a bug and then worked backwards to create a *minimal* bugfix that applies to the current code as well as older stable branches. Obviously this is more of a concern for stable fixes than for new code. But we have to keep in mind that every code churn will make future bug fixes harder to roll out to users. That is not to say that churn should be avoided, just that we need to balance a change's benefit against this cost. > And always keeping the user interface and even the code unchanged > contradicts my motivation of contributing to the Linux kernel. All my > contributions are motivated by the hope to clean things up. I'm not an > actual user of any of the code I contribute. If we adhere to the > philosophy of not doing any clean-ups, my contributions would be > meaningless. There are cleanups and cleanups. Dead code removal and deduplication of open coded logic, for instance, are very valuable. As is, for instance, your work in making sense of hard_header_len. Returning code in branches vs an error jump label seems more of a personal preference, and to me does not pass the benefit/cost threshold. FWIW, there is lots of code that I would jump at the opportunity to restructure. Starting with skb_segment, probably. Obviously, all this is just one opinion on the topic.
On Sat, 31 Oct 2020 15:47:28 -0400 Willem de Bruijn wrote: > On Sat, Oct 31, 2020 at 12:02 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Sat, Oct 31, 2020 at 8:18 AM Xie He <xie.he.0141@gmail.com> wrote: > > > > Especially without that, I'm not sure this and the follow-on patch add > > > > much value. Minor code cleanups complicate backports of fixes. > > > > > > To me this is necessary, because I feel hard to do any development on > > > un-cleaned-up code. I really don't know how to add my code without > > > these clean-ups, and even if I managed to do that, I would not be > > > happy with my code. > > That is the reality of working in this space, I think. I have > frequently restructured code, fixed a bug and then worked backwards to > create a *minimal* bugfix that applies to the current code as well as > older stable branches. > > Obviously this is more of a concern for stable fixes than for new > code. But we have to keep in mind that every code churn will make > future bug fixes harder to roll out to users. That is not to say that > churn should be avoided, just that we need to balance a change's > benefit against this cost. > > > And always keeping the user interface and even the code unchanged > > contradicts my motivation of contributing to the Linux kernel. All my > > contributions are motivated by the hope to clean things up. I'm not an > > actual user of any of the code I contribute. If we adhere to the > > philosophy of not doing any clean-ups, my contributions would be > > meaningless. > > There are cleanups and cleanups. Dead code removal and deduplication > of open coded logic, for instance, are very valuable. As is, for > instance, your work in making sense of hard_header_len. Or removing the buggy uses of IFF_TX_SKB_SHARING, for that matter (which at this point I agree we should just remove from ether_setup, and let people who care re-enable it). > Returning code in branches vs an error jump label seems more of a > personal preference, and to me does not pass the benefit/cost threshold. I must agree.
On Sat, Oct 31, 2020 at 12:48 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > Returning code in branches vs an error jump label seems more of a > personal preference, and to me does not pass the benefit/cost threshold. This patch is necessary for the 2nd and 5th patch in this series, because the 2nd and 5th patch would add a lot of places where we need to "error out" and drop the frame. Without this patch, the 2nd and 5th patch would add a lot of useless code. The 2nd patch is also necessary for the 5th patch, because otherwise I would not know how to produce the 5th patch. The logic is so convoluted for me. And it seems to me that the simplest way for me would make all code to follow the logic of eth_type_trans. The patch series was actually a single patch previously: http://patchwork.ozlabs.org/project/netdev/patch/20201017051951.363514-1-xie.he.0141@gmail.com/ I splitted it to make changes I do clearer. But really these patches should be as a whole. It's really hard for me to do the 5th patch without the 1st and 2nd patch.
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c index 409e5a7ad8e2..4db0e01b96a9 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,12 @@ 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: dev_kfree_skb_any(skb); return NET_RX_DROP; }
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. 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 | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)