From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Schiller Subject: Re: [PATCH net-next RFC v2] net: hdlc_x25: Queue outgoing LAPB frames Date: Mon, 15 Feb 2021 07:27:35 +0100 Message-ID: References: <20210210173532.370914-1-xie.he.0141@gmail.com> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20210210173532.370914-1-xie.he.0141@gmail.com> List-ID: Content-Type: text/plain; charset="us-ascii"; format="flowed" To: Xie He Cc: "David S. Miller" , Jakub Kicinski , linux-x25@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Krzysztof Halasa On 2021-02-10 18:35, Xie He wrote: > When sending packets, we will first hand over the (L3) packets to the > LAPB module, then the LAPB module will hand over the corresponding LAPB > (L2) frames back to us for us to transmit. > > The LAPB module can also emit LAPB (L2) frames at any time without an > (L3) packet currently being sent, when it is trying to send (L3) > packets > queued up in its internal queue, or when it decides to send some (L2) > control frame. > > This means we need have a queue for these outgoing LAPB (L2) frames to > avoid congestion. This queue needs to be controlled by the hardware > drivers' netif_stop_queue and netif_wake_queue calls. So we need to use > a qdisc TX queue for this purpose. > > On the other hand, the outgoing (L3) packets don't need to be queued, > because the LAPB module already has an internal queue for them. > > However, currently the outgoing LAPB (L2) frames are not queued. This > can cause frames to be dropped by hardware drivers when they are busy. > At the same time the (L3) packets are being queued and controlled by > hardware drivers' netif_stop_queue and netif_wake_queue calls. This is > unnecessary and meaningless. > > To solve this problem, we can split the HDLC device into two devices: > a virtual X.25 device and an actual HDLC device, using the virtual > X.25 device to send (L3) packets and using the actual HDLC device to > queue LAPB (L2) frames. The outgoing LAPB queue will be controlled by > hardware drivers' netif_stop_queue and netif_wake_queue calls, while > outgoing (L3) packets will not be affected by these calls. At first glance, the patch looks quite reasonable. The only thing I noticed right away is that you also included the changes of your patch "Return meaningful error code in x25_open". I hope to get back to the office this week and test it. > > Cc: Martin Schiller > Signed-off-by: Xie He > --- > > Change from RFC v1: > Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock. > > --- > drivers/net/wan/hdlc_x25.c | 158 ++++++++++++++++++++++++++++++------- > 1 file changed, 129 insertions(+), 29 deletions(-) > > diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c > index bb164805804e..2a7b3c3d0c05 100644 > --- a/drivers/net/wan/hdlc_x25.c > +++ b/drivers/net/wan/hdlc_x25.c > @@ -23,6 +23,13 @@ > > struct x25_state { > x25_hdlc_proto settings; > + struct net_device *x25_dev; > + spinlock_t x25_dev_lock; /* Protects the x25_dev pointer */ > +}; > + > +/* Pointed to by netdev_priv(x25_dev) */ > +struct x25_device { > + struct net_device *hdlc_dev; > }; > > static int x25_ioctl(struct net_device *dev, struct ifreq *ifr); > @@ -32,6 +39,11 @@ static struct x25_state *state(hdlc_device *hdlc) > return hdlc->state; > } > > +static struct x25_device *dev_to_x25(struct net_device *dev) > +{ > + return netdev_priv(dev); > +} > + > /* These functions are callbacks called by LAPB layer */ > > static void x25_connect_disconnect(struct net_device *dev, int > reason, int code) > @@ -89,15 +101,10 @@ static int x25_data_indication(struct net_device > *dev, struct sk_buff *skb) > > static void x25_data_transmit(struct net_device *dev, struct sk_buff > *skb) > { > - hdlc_device *hdlc = dev_to_hdlc(dev); > - > + skb->dev = dev_to_x25(dev)->hdlc_dev; > + skb->protocol = htons(ETH_P_HDLC); > skb_reset_network_header(skb); > - skb->protocol = hdlc_type_trans(skb, dev); > - > - if (dev_nit_active(dev)) > - dev_queue_xmit_nit(skb, dev); > - > - hdlc->xmit(skb, dev); /* Ignore return value :-( */ > + dev_queue_xmit(skb); > } > > > @@ -163,17 +170,18 @@ static int x25_open(struct net_device *dev) > .data_indication = x25_data_indication, > .data_transmit = x25_data_transmit, > }; > - hdlc_device *hdlc = dev_to_hdlc(dev); > + struct net_device *hdlc_dev = dev_to_x25(dev)->hdlc_dev; > + hdlc_device *hdlc = dev_to_hdlc(hdlc_dev); > struct lapb_parms_struct params; > int result; > > result = lapb_register(dev, &cb); > if (result != LAPB_OK) > - return result; > + return -ENOMEM; > > result = lapb_getparms(dev, ¶ms); > if (result != LAPB_OK) > - return result; > + return -EINVAL; > > if (state(hdlc)->settings.dce) > params.mode = params.mode | LAPB_DCE; > @@ -188,16 +196,104 @@ static int x25_open(struct net_device *dev) > > result = lapb_setparms(dev, ¶ms); > if (result != LAPB_OK) > - return result; > + return -EINVAL; > > return 0; > } > > > > -static void x25_close(struct net_device *dev) > +static int x25_close(struct net_device *dev) > { > lapb_unregister(dev); > + return 0; > +} > + > +static const struct net_device_ops hdlc_x25_netdev_ops = { > + .ndo_open = x25_open, > + .ndo_stop = x25_close, > + .ndo_start_xmit = x25_xmit, > +}; > + > +static void x25_setup_virtual_dev(struct net_device *dev) > +{ > + dev->netdev_ops = &hdlc_x25_netdev_ops; > + dev->type = ARPHRD_X25; > + dev->addr_len = 0; > + dev->hard_header_len = 0; > +} > + > +static int x25_hdlc_open(struct net_device *dev) > +{ > + struct hdlc_device *hdlc = dev_to_hdlc(dev); > + struct net_device *x25_dev; > + char x25_dev_name[sizeof(x25_dev->name)]; > + int result; > + > + if (strlen(dev->name) + 4 >= sizeof(x25_dev_name)) > + return -EINVAL; > + > + strcpy(x25_dev_name, dev->name); > + strcat(x25_dev_name, "_x25"); > + > + x25_dev = alloc_netdev(sizeof(struct x25_device), x25_dev_name, > + NET_NAME_PREDICTABLE, x25_setup_virtual_dev); > + if (!x25_dev) > + return -ENOMEM; > + > + /* When transmitting data: > + * first we'll remove a pseudo header of 1 byte, > + * then the LAPB module will prepend an LAPB header of at most 3 > bytes. > + */ > + x25_dev->needed_headroom = 3 - 1; > + x25_dev->mtu = dev->mtu - (3 - 1); > + dev_to_x25(x25_dev)->hdlc_dev = dev; > + > + result = register_netdevice(x25_dev); > + if (result) { > + free_netdev(x25_dev); > + return result; > + } > + > + spin_lock_bh(&state(hdlc)->x25_dev_lock); > + state(hdlc)->x25_dev = x25_dev; > + spin_unlock_bh(&state(hdlc)->x25_dev_lock); > + > + return 0; > +} > + > +static void x25_hdlc_close(struct net_device *dev) > +{ > + struct hdlc_device *hdlc = dev_to_hdlc(dev); > + struct net_device *x25_dev = state(hdlc)->x25_dev; > + > + if (x25_dev->flags & IFF_UP) > + dev_close(x25_dev); > + > + spin_lock_bh(&state(hdlc)->x25_dev_lock); > + state(hdlc)->x25_dev = NULL; > + spin_unlock_bh(&state(hdlc)->x25_dev_lock); > + > + unregister_netdevice(x25_dev); > + free_netdev(x25_dev); > +} > + > +static void x25_hdlc_start(struct net_device *dev) > +{ > + struct hdlc_device *hdlc = dev_to_hdlc(dev); > + struct net_device *x25_dev = state(hdlc)->x25_dev; > + > + /* hdlc.c guarantees no racing so we're sure x25_dev is valid */ > + netif_carrier_on(x25_dev); > +} > + > +static void x25_hdlc_stop(struct net_device *dev) > +{ > + struct hdlc_device *hdlc = dev_to_hdlc(dev); > + struct net_device *x25_dev = state(hdlc)->x25_dev; > + > + /* hdlc.c guarantees no racing so we're sure x25_dev is valid */ > + netif_carrier_off(x25_dev); > } > > > @@ -205,27 +301,38 @@ static void x25_close(struct net_device *dev) > static int x25_rx(struct sk_buff *skb) > { > struct net_device *dev = skb->dev; > + struct hdlc_device *hdlc = dev_to_hdlc(dev); > + struct net_device *x25_dev; > > if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) { > dev->stats.rx_dropped++; > return NET_RX_DROP; > } > > - if (lapb_data_received(dev, skb) == LAPB_OK) > - return NET_RX_SUCCESS; > - > - dev->stats.rx_errors++; > + spin_lock_bh(&state(hdlc)->x25_dev_lock); > + x25_dev = state(hdlc)->x25_dev; > + if (!x25_dev) > + goto drop; > + if (lapb_data_received(x25_dev, skb) != LAPB_OK) > + goto drop; > + spin_unlock_bh(&state(hdlc)->x25_dev_lock); > + return NET_RX_SUCCESS; > + > +drop: > + spin_unlock_bh(&state(hdlc)->x25_dev_lock); > + dev->stats.rx_dropped++; > dev_kfree_skb_any(skb); > return NET_RX_DROP; > } > > > static struct hdlc_proto proto = { > - .open = x25_open, > - .close = x25_close, > + .open = x25_hdlc_open, > + .close = x25_hdlc_close, > + .start = x25_hdlc_start, > + .stop = x25_hdlc_stop, > .ioctl = x25_ioctl, > .netif_rx = x25_rx, > - .xmit = x25_xmit, > .module = THIS_MODULE, > }; > > @@ -298,16 +405,9 @@ static int x25_ioctl(struct net_device *dev, > struct ifreq *ifr) > return result; > > memcpy(&state(hdlc)->settings, &new_settings, size); > + state(hdlc)->x25_dev = NULL; > + spin_lock_init(&state(hdlc)->x25_dev_lock); > > - /* There's no header_ops so hard_header_len should be 0. */ > - dev->hard_header_len = 0; > - /* When transmitting data: > - * first we'll remove a pseudo header of 1 byte, > - * then we'll prepend an LAPB header of at most 3 bytes. > - */ > - dev->needed_headroom = 3 - 1; > - > - dev->type = ARPHRD_X25; > call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev); > netif_dormant_off(dev); > return 0;