| Message ID | 20210210173532.370914-1-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RFC,v2] net: hdlc_x25: Queue outgoing LAPB frames | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 232 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
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 <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> > --- > > 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;
On Sun, Feb 14, 2021 at 10:27 PM Martin Schiller <ms@dev.tdt.de> wrote: > > 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". Thanks! It was because this patch was sent before that fix got merged into "net-next". I will drop that part now. I will also make the MTU of the virtual X.25 device be HDLC_MAX_MTU (instead of HDLC_MAX_MTU - 2), because I see other HDLC Protocol Drivers seem to also use this value as MTU (without subtracting the header length). > I hope to get back to the office this week and test it. Thanks!
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;
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. Cc: Martin Schiller <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- 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(-)