| Message ID | 20210127090747.364951-1-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: hdlc_x25: Use qdisc to 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 |
| 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, 30 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 |
From: Xie He > Sent: 27 January 2021 09:08 > > An HDLC hardware driver may call netif_stop_queue to temporarily stop > the TX queue when the hardware is busy sending a frame, and after the > hardware has finished sending the frame, call netif_wake_queue to > resume the TX queue. > > However, the LAPB module doesn't know about this. Whether or not the > hardware driver has stopped the TX queue, the LAPB module still feeds > outgoing frames to the hardware driver for transmission. This can cause > frames to be dropped by the hardware driver. > > It's not easy to fix this issue in the LAPB module. We can indeed let the > LAPB module check whether the TX queue has been stopped before feeding > each frame to the hardware driver, but when the hardware driver resumes > the TX queue, it's not easy to immediately notify the LAPB module and ask > it to resume transmission. > > Instead, we can fix this issue at the hdlc_x25 layer, by using qdisc TX > queues to queue outgoing LAPB frames. The qdisc TX queue will then > automatically be controlled by netif_stop_queue and netif_wake_queue. > > This way, when sending, we will use the qdisc queue to queue and send > the data twice: once as the L3 packet and then (after processed by the > LAPB module) as an LAPB (L2) frame. This does not make the logic of the > code messy, because when receiving, data are already "received" on the > device twice: once as an LAPB (L2) frame and then (after processed by > the LAPB module) as the L3 packet. If I read this correctly it adds a (potentially big) queue between the LAPB code that adds the sequence numbers to the frames and the hardware that actually sends them. IIRC [1] there is a general expectation that the NR in a transmitted frame will be the same as the last received NS unless acks are being delayed for flow control reasons. You definitely want to be able to ack a received frame while transmitting back-to-back I-frames. This really means that you only want 2 frames in the hardware driver. The one being transmitted and the next one - so it gets sent with a shared flag. There is no point sending an RR unless the hardware link is actually idle. [1] I've been doing to much SS7 MTP2 recently, I can't quite remember all of LAPB! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Jan 27, 2021 at 2:14 AM David Laight <David.Laight@aculab.com> wrote: > > If I read this correctly it adds a (potentially big) queue between the > LAPB code that adds the sequence numbers to the frames and the hardware > that actually sends them. Yes. The actual number of outgoing LAPB frames being queued depends on how long the hardware driver stays in the TX busy state, and is limited by the LAPB sending window. > IIRC [1] there is a general expectation that the NR in a transmitted frame > will be the same as the last received NS unless acks are being delayed > for flow control reasons. > > You definitely want to be able to ack a received frame while transmitting > back-to-back I-frames. > > This really means that you only want 2 frames in the hardware driver. > The one being transmitted and the next one - so it gets sent with a > shared flag. > There is no point sending an RR unless the hardware link is actually idle. If I understand correctly, what you mean is that the frames sent on the wire should reflect the most up-to-date status of what is received from the wire, so queueing outgoing LAPB frames is not appropriate. But this would require us to deal with the "TX busy" issue in the LAPB module. This is (as I said) not easy to do. I currently can't think of a good way of doing this. Instead, we can think of the TX queue as part of the "wire". We can think of the wire as long and having a little higher latency. I believe the LAPB protocol has no problem in handling long wires. What do you think?
On 2021-01-27 21:29, Xie He wrote: > On Wed, Jan 27, 2021 at 2:14 AM David Laight <David.Laight@aculab.com> > wrote: >> >> If I read this correctly it adds a (potentially big) queue between the >> LAPB code that adds the sequence numbers to the frames and the >> hardware >> that actually sends them. > > Yes. The actual number of outgoing LAPB frames being queued depends on > how long the hardware driver stays in the TX busy state, and is > limited by the LAPB sending window. > >> IIRC [1] there is a general expectation that the NR in a transmitted >> frame >> will be the same as the last received NS unless acks are being delayed >> for flow control reasons. >> >> You definitely want to be able to ack a received frame while >> transmitting >> back-to-back I-frames. >> >> This really means that you only want 2 frames in the hardware driver. >> The one being transmitted and the next one - so it gets sent with a >> shared flag. >> There is no point sending an RR unless the hardware link is actually >> idle. > > If I understand correctly, what you mean is that the frames sent on > the wire should reflect the most up-to-date status of what is received > from the wire, so queueing outgoing LAPB frames is not appropriate. > > But this would require us to deal with the "TX busy" issue in the LAPB > module. This is (as I said) not easy to do. I currently can't think of > a good way of doing this. > > Instead, we can think of the TX queue as part of the "wire". We can > think of the wire as long and having a little higher latency. I > believe the LAPB protocol has no problem in handling long wires. > > What do you think? David: Can you please elaborate on your concerns a little bit more? I think Xie's approach is not bad at all. LAPB (L2) has no idea about L1 (apart from the link state) and sends as many packets as possible, which of course we should not discard. The remaining window determines how many packets are put into this queue. Since we can't send anything over the line due to the TX Busy state, the remote station (due to lack of ACKs) will also stop sending anything at some point. When the link goes down, all buffers/queues must be cleared.
On Wed, 27 Jan 2021 01:07:47 -0800 Xie He wrote: > An HDLC hardware driver may call netif_stop_queue to temporarily stop > the TX queue when the hardware is busy sending a frame, and after the > hardware has finished sending the frame, call netif_wake_queue to > resume the TX queue. > > However, the LAPB module doesn't know about this. Whether or not the > hardware driver has stopped the TX queue, the LAPB module still feeds > outgoing frames to the hardware driver for transmission. This can cause > frames to be dropped by the hardware driver. > > It's not easy to fix this issue in the LAPB module. We can indeed let the > LAPB module check whether the TX queue has been stopped before feeding > each frame to the hardware driver, but when the hardware driver resumes > the TX queue, it's not easy to immediately notify the LAPB module and ask > it to resume transmission. > > Instead, we can fix this issue at the hdlc_x25 layer, by using qdisc TX > queues to queue outgoing LAPB frames. The qdisc TX queue will then > automatically be controlled by netif_stop_queue and netif_wake_queue. Noob question - could you point at or provide a quick guide to layering here? I take there is only one netdev, and something maintains an internal queue which is not stopped when HW driver stops the qdisc? > This way, when sending, we will use the qdisc queue to queue and send > the data twice: once as the L3 packet and then (after processed by the > LAPB module) as an LAPB (L2) frame. This does not make the logic of the > code messy, because when receiving, data are already "received" on the > device twice: once as an LAPB (L2) frame and then (after processed by > the LAPB module) as the L3 packet. > > Some more details about the code change: > > 1. dev_queue_xmit_nit is removed because we already have it when we send > the skb through the qdisc TX queue (in xmit_one). > > 2. hdlc_type_trans is replaced by assigning skb->dev and skb->protocol > directly. skb_reset_mac_header in hdlc_type_trans is no longer necessary > because it will be called in __dev_queue_xmit. Sounds like we're optimizing to prevent drops, and this was not reported from production, rather thru code inspection. Ergo I think net-next will be more appropriate here, unless Martin disagrees. > diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c > index bb164805804e..b7f2823bf100 100644 > --- a/drivers/net/wan/hdlc_x25.c > +++ b/drivers/net/wan/hdlc_x25.c > @@ -89,15 +89,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; > + 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); > } > > > @@ -106,6 +101,12 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev) > { > int result; > > + if (skb->protocol == htons(ETH_P_HDLC)) { > + hdlc_device *hdlc = dev_to_hdlc(dev); > + > + return hdlc->xmit(skb, dev); > + } > + > /* There should be a pseudo header of 1 byte added by upper layers. > * Check to make sure it is there before reading it. > */
On Thu, Jan 28, 2021 at 11:47 AM Jakub Kicinski <kuba@kernel.org> wrote: > > Noob question - could you point at or provide a quick guide to layering > here? I take there is only one netdev, and something maintains an > internal queue which is not stopped when HW driver stops the qdisc? Yes, there is only one netdev. The LAPB module (net/lapb/) (which is used as a library by the netdev driver - hdlc_x25.c) is maintaining an internal queue which is not stopped when the HW driver stops the qdisc. The queue is "write_queue" in "struct lapb_cb" in "include/net/lapb.h". The code that takes skbs out of the queue and feeds them to lower layers for transmission is at the "lapb_kick" function in "net/lapb/lapb_out.c". The layering is like this: Upper layer (Layer 3) (net/x25/ or net/packet/) ^ | L3 packets (with control info) v The netdev driver (hdlc_x25.c) ^ | L3 packets v The LAPB Module (net/lapb/) ^ | LAPB (L2) frames v The netdev driver (hdlc_x25.c) ^ | LAPB (L2) frames | (also called HDLC frames in the context of the HDLC subsystem) v HDLC core (hdlc.c) ^ | HDLC frames v HDLC Hardware Driver > Sounds like we're optimizing to prevent drops, and this was not > reported from production, rather thru code inspection. Ergo I think > net-next will be more appropriate here, unless Martin disagrees. Yes, I have no problem in targeting net-next instead. Thanks!
On 2021-01-28 23:06, Xie He wrote: > On Thu, Jan 28, 2021 at 11:47 AM Jakub Kicinski <kuba@kernel.org> > wrote: >> >> Noob question - could you point at or provide a quick guide to >> layering >> here? I take there is only one netdev, and something maintains an >> internal queue which is not stopped when HW driver stops the qdisc? > > Yes, there is only one netdev. The LAPB module (net/lapb/) (which is > used as a library by the netdev driver - hdlc_x25.c) is maintaining an > internal queue which is not stopped when the HW driver stops the > qdisc. > > The queue is "write_queue" in "struct lapb_cb" in > "include/net/lapb.h". The code that takes skbs out of the queue and > feeds them to lower layers for transmission is at the "lapb_kick" > function in "net/lapb/lapb_out.c". > > The layering is like this: > > Upper layer (Layer 3) (net/x25/ or net/packet/) > > ^ > | L3 packets (with control info) > v > > The netdev driver (hdlc_x25.c) > > ^ > | L3 packets > v > > The LAPB Module (net/lapb/) > > ^ > | LAPB (L2) frames > v > > The netdev driver (hdlc_x25.c) > > ^ > | LAPB (L2) frames > | (also called HDLC frames in the context of the HDLC subsystem) > v > > HDLC core (hdlc.c) > > ^ > | HDLC frames > v > > HDLC Hardware Driver @Xie: Thank you for the detailed presentation. > >> Sounds like we're optimizing to prevent drops, and this was not >> reported from production, rather thru code inspection. Ergo I think >> net-next will be more appropriate here, unless Martin disagrees. > > Yes, I have no problem in targeting net-next instead. Thanks! I agree.
On Fri, 29 Jan 2021 06:56:10 +0100 Martin Schiller wrote: > On 2021-01-28 23:06, Xie He wrote: > > On Thu, Jan 28, 2021 at 11:47 AM Jakub Kicinski <kuba@kernel.org> > > wrote: > >> > >> Noob question - could you point at or provide a quick guide to > >> layering > >> here? I take there is only one netdev, and something maintains an > >> internal queue which is not stopped when HW driver stops the qdisc? > > > > Yes, there is only one netdev. The LAPB module (net/lapb/) (which is > > used as a library by the netdev driver - hdlc_x25.c) is maintaining an > > internal queue which is not stopped when the HW driver stops the > > qdisc. > > > > The queue is "write_queue" in "struct lapb_cb" in > > "include/net/lapb.h". The code that takes skbs out of the queue and > > feeds them to lower layers for transmission is at the "lapb_kick" > > function in "net/lapb/lapb_out.c". > > > > The layering is like this: > > > > Upper layer (Layer 3) (net/x25/ or net/packet/) > > > > ^ > > | L3 packets (with control info) > > v > > > > The netdev driver (hdlc_x25.c) > > > > ^ > > | L3 packets > > v > > > > The LAPB Module (net/lapb/) > > > > ^ > > | LAPB (L2) frames > > v > > > > The netdev driver (hdlc_x25.c) > > > > ^ > > | LAPB (L2) frames > > | (also called HDLC frames in the context of the HDLC subsystem) > > v > > > > HDLC core (hdlc.c) > > > > ^ > > | HDLC frames > > v > > > > HDLC Hardware Driver > > @Xie: Thank you for the detailed presentation. Indeed, thanks for the diagram. I'm still struggling to wrap my head around this. Did you test your code with lockdep enabled? Which Qdisc are you using? You're queuing the frames back to the interface they came from - won't that cause locking issues?
On Fri, Jan 29, 2021 at 5:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > I'm still struggling to wrap my head around this. > > Did you test your code with lockdep enabled? Which Qdisc are you using? > You're queuing the frames back to the interface they came from - won't > that cause locking issues? Hmm... Thanks for bringing this to my attention. I indeed find issues when the "noqueue" qdisc is used. When using a qdisc other than "noqueue", when sending an skb: "__dev_queue_xmit" will call "__dev_xmit_skb"; "__dev_xmit_skb" will call "qdisc_run_begin" to mark the beginning of a qdisc run, and if the qdisc is already running, "qdisc_run_begin" will fail, then "__dev_xmit_skb" will just enqueue this skb without starting qdisc. There is no problem. When using "noqueue" as the qdisc, when sending an skb: "__dev_queue_xmit" will try to send this skb directly. Before it does that, it will first check "txq->xmit_lock_owner" and will find that the current cpu already owns the xmit lock, it will then print a warning message "Dead loop on virtual device ..." and drop the skb. A solution can be queuing the outgoing L2 frames in this driver first, and then using a tasklet to send them to the qdisc TX queue. Thanks! I'll make changes to fix this.
On Sat, 30 Jan 2021 06:29:20 -0800 Xie He wrote: > On Fri, Jan 29, 2021 at 5:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > I'm still struggling to wrap my head around this. > > > > Did you test your code with lockdep enabled? Which Qdisc are you using? > > You're queuing the frames back to the interface they came from - won't > > that cause locking issues? > > Hmm... Thanks for bringing this to my attention. I indeed find issues > when the "noqueue" qdisc is used. > > When using a qdisc other than "noqueue", when sending an skb: > "__dev_queue_xmit" will call "__dev_xmit_skb"; > "__dev_xmit_skb" will call "qdisc_run_begin" to mark the beginning of > a qdisc run, and if the qdisc is already running, "qdisc_run_begin" > will fail, then "__dev_xmit_skb" will just enqueue this skb without > starting qdisc. There is no problem. > > When using "noqueue" as the qdisc, when sending an skb: > "__dev_queue_xmit" will try to send this skb directly. Before it does > that, it will first check "txq->xmit_lock_owner" and will find that > the current cpu already owns the xmit lock, it will then print a > warning message "Dead loop on virtual device ..." and drop the skb. > > A solution can be queuing the outgoing L2 frames in this driver first, > and then using a tasklet to send them to the qdisc TX queue. > > Thanks! I'll make changes to fix this. Sounds like too much afford for a sub-optimal workaround. The qdisc semantics are borken in the proposed scheme (double counting packets) - both in term of statistics and if user decides to add a policer, filter etc. Another worry is that something may just inject a packet with skb->protocol == ETH_P_HDLC but unexpected structure (IDK if that's a real concern). It may be better to teach LAPB to stop / start the internal queue. The lower level drivers just needs to call LAPB instead of making the start/wake calls directly to the stack, and LAPB can call the stack. Would that not work?
On Sat, Jan 30, 2021 at 11:16 AM Jakub Kicinski <kuba@kernel.org> wrote: > > Sounds like too much afford for a sub-optimal workaround. > The qdisc semantics are borken in the proposed scheme (double > counting packets) - both in term of statistics and if user decides > to add a policer, filter etc. Hmm... Another solution might be creating another virtual device on top of the HDLC device (similar to what "hdlc_fr.c" does), so that we can first queue L3 packets in the virtual device's qdisc queue, and then queue the L2 frames in the actual HDLC device's qdisc queue. This way we can avoid the same outgoing data being queued to qdisc twice. But this would significantly change the way the user uses the hdlc_x25 driver. > Another worry is that something may just inject a packet with > skb->protocol == ETH_P_HDLC but unexpected structure (IDK if > that's a real concern). This might not be a problem. Ethernet devices also allow the user to inject raw frames with user constructed headers. "hdlc_fr.c" also allows the user to bypass the virtual circuit interfaces and inject raw frames directly on the HDLC interface. I think the receiving side should be able to recognize and drop invalid frames. > It may be better to teach LAPB to stop / start the internal queue. > The lower level drivers just needs to call LAPB instead of making > the start/wake calls directly to the stack, and LAPB can call the > stack. Would that not work? I think this is a good solution. But this requires changing a lot of code. The HDLC subsystem needs to be changed to allow HDLC Hardware Drivers to ask HDLC Protocol Drivers (like hdlc_x25.c) to stop/wake the TX queue. The hdlc_x25.c driver can then ask the LAPB module to stop/wake the queue. So this means new APIs need to be added to both the HDLC subsystem and the LAPB module, and a number of HDLC Hardware Drivers need to be changed to call the new API of the HDLC subsystem. Martin, do you have any suggestions?
On 2021-01-31 04:16, Xie He wrote: > On Sat, Jan 30, 2021 at 11:16 AM Jakub Kicinski <kuba@kernel.org> > wrote: >> >> Sounds like too much afford for a sub-optimal workaround. >> The qdisc semantics are borken in the proposed scheme (double >> counting packets) - both in term of statistics and if user decides >> to add a policer, filter etc. > > Hmm... > > Another solution might be creating another virtual device on top of > the HDLC device (similar to what "hdlc_fr.c" does), so that we can > first queue L3 packets in the virtual device's qdisc queue, and then > queue the L2 frames in the actual HDLC device's qdisc queue. This way > we can avoid the same outgoing data being queued to qdisc twice. But > this would significantly change the way the user uses the hdlc_x25 > driver. > >> Another worry is that something may just inject a packet with >> skb->protocol == ETH_P_HDLC but unexpected structure (IDK if >> that's a real concern). > > This might not be a problem. Ethernet devices also allow the user to > inject raw frames with user constructed headers. "hdlc_fr.c" also > allows the user to bypass the virtual circuit interfaces and inject > raw frames directly on the HDLC interface. I think the receiving side > should be able to recognize and drop invalid frames. > >> It may be better to teach LAPB to stop / start the internal queue. >> The lower level drivers just needs to call LAPB instead of making >> the start/wake calls directly to the stack, and LAPB can call the >> stack. Would that not work? > > I think this is a good solution. But this requires changing a lot of > code. The HDLC subsystem needs to be changed to allow HDLC Hardware > Drivers to ask HDLC Protocol Drivers (like hdlc_x25.c) to stop/wake > the TX queue. The hdlc_x25.c driver can then ask the LAPB module to > stop/wake the queue. > > So this means new APIs need to be added to both the HDLC subsystem and > the LAPB module, and a number of HDLC Hardware Drivers need to be > changed to call the new API of the HDLC subsystem. > > Martin, do you have any suggestions? I have thought about this issue again. I also have to say that I have never noticed any problems in this area before. So again for (my) understanding: When a hardware driver calls netif_stop_queue, the frames sent from layer 3 (X.25) with dev_queue_xmit are queued and not passed "directly" to x25_xmit of the hdlc_x25 driver. So nothing is added to the write_queue anymore (except possibly un-acked-frames by lapb_requeue_frames). Shouldn't it actually be sufficient to check for netif_queue_stopped in lapb_kick and then do "nothing" if necessary? As soon as the hardware driver calls netif_wake_queue, the whole thing should just continue running. Or am I missing something?
On Mon, Feb 1, 2021 at 1:18 AM Martin Schiller <ms@dev.tdt.de> wrote: > > I have thought about this issue again. > > I also have to say that I have never noticed any problems in this area > before. > > So again for (my) understanding: > When a hardware driver calls netif_stop_queue, the frames sent from > layer 3 (X.25) with dev_queue_xmit are queued and not passed "directly" > to x25_xmit of the hdlc_x25 driver. > > So nothing is added to the write_queue anymore (except possibly > un-acked-frames by lapb_requeue_frames). If the LAPB module only emits an L2 frame when an L3 packet comes from the upper layer, then yes, there would be no problem because the L3 packet is already controlled by the qdisc and there is no need to control the corresponding L2 frame again. However, the LAPB module can emits L2 frames when there's no L3 packet coming, when 1) there are some packets queued in the LAPB module's internal queue; and 2) the LAPB decides to send some control frame (e.g. by the timers). > Shouldn't it actually be sufficient to check for netif_queue_stopped in > lapb_kick and then do "nothing" if necessary? We can consider this situation: When the upper layer has nothing to send, but there are some packets in the LAPB module's internal queue waiting to be sent. The LAPB module will try to send the packets, but after it has sent out the first packet, it will meet the "queue stopped" situation. In this situation, it'd be preferable to immediately start sending the second packet after the queue is started again. "Doing nothing" in this situation would mean waiting until some other events occur, such as receiving responses from the other side, or receiving more outgoing packets from L3. > As soon as the hardware driver calls netif_wake_queue, the whole thing > should just continue running. This relies on the fact that the upper layer has something to send. If the upper layer has nothing to send, lapb_kick would not be automatically called again until some other events occur (such as receiving responses from the other side). I think it'd be better if we do not rely on the assumption that L3 is going to send more packets to us, as L3 itself would assume us to provide it a reliable link service and we should fulfill its expectation.
On 2021-02-01 12:38, Xie He wrote: > On Mon, Feb 1, 2021 at 1:18 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> I have thought about this issue again. >> >> I also have to say that I have never noticed any problems in this area >> before. >> >> So again for (my) understanding: >> When a hardware driver calls netif_stop_queue, the frames sent from >> layer 3 (X.25) with dev_queue_xmit are queued and not passed >> "directly" >> to x25_xmit of the hdlc_x25 driver. >> >> So nothing is added to the write_queue anymore (except possibly >> un-acked-frames by lapb_requeue_frames). > > If the LAPB module only emits an L2 frame when an L3 packet comes from > the upper layer, then yes, there would be no problem because the L3 > packet is already controlled by the qdisc and there is no need to > control the corresponding L2 frame again. > > However, the LAPB module can emits L2 frames when there's no L3 packet > coming, when 1) there are some packets queued in the LAPB module's > internal queue; and 2) the LAPB decides to send some control frame > (e.g. by the timers). But control frames are currently sent past the lapb write_queue. So another queue would have to be created. And wouldn't it be better to have it in the hdlc_x25 driver, leaving LAPB unaffected? > >> Shouldn't it actually be sufficient to check for netif_queue_stopped >> in >> lapb_kick and then do "nothing" if necessary? > > We can consider this situation: When the upper layer has nothing to > send, but there are some packets in the LAPB module's internal queue > waiting to be sent. The LAPB module will try to send the packets, but > after it has sent out the first packet, it will meet the "queue > stopped" situation. In this situation, it'd be preferable to > immediately start sending the second packet after the queue is started > again. "Doing nothing" in this situation would mean waiting until some > other events occur, such as receiving responses from the other side, > or receiving more outgoing packets from L3. > >> As soon as the hardware driver calls netif_wake_queue, the whole thing >> should just continue running. > > This relies on the fact that the upper layer has something to send. If > the upper layer has nothing to send, lapb_kick would not be > automatically called again until some other events occur (such as > receiving responses from the other side). I think it'd be better if we > do not rely on the assumption that L3 is going to send more packets to > us, as L3 itself would assume us to provide it a reliable link service > and we should fulfill its expectation.
On Mon, Feb 1, 2021 at 5:14 AM Martin Schiller <ms@dev.tdt.de> wrote: > > But control frames are currently sent past the lapb write_queue. > So another queue would have to be created. > > And wouldn't it be better to have it in the hdlc_x25 driver, leaving > LAPB unaffected? Hmm.. Indeed. I agree. I also think the queue needs to be the qdisc queue, so that it'll be able to respond immediately to hardware drivers' netif_wake_queue call. Initially I was considering using the qdisc of the HDLC device to queue the outgoing L2 frames again (after their corresponding L3 packets having already gone through the queue). But Jakub didn't like the idea of queuing the same data twice. I also found that if an L3 packet was sent through the qdisc without being queued, and LAPB didn't queue it either, then the emitted L2 frame must be queued in the qdisc. This is both not optimal and causing problems when using the "noqueue" qdisc. Maybe the only way is to create a virtual device on top of the HDLC device, using the virtual device to queue L3 packets and using the actual HDLC device to queue L2 frames.
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c index bb164805804e..b7f2823bf100 100644 --- a/drivers/net/wan/hdlc_x25.c +++ b/drivers/net/wan/hdlc_x25.c @@ -89,15 +89,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; + 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); } @@ -106,6 +101,12 @@ static netdev_tx_t x25_xmit(struct sk_buff *skb, struct net_device *dev) { int result; + if (skb->protocol == htons(ETH_P_HDLC)) { + hdlc_device *hdlc = dev_to_hdlc(dev); + + return hdlc->xmit(skb, dev); + } + /* There should be a pseudo header of 1 byte added by upper layers. * Check to make sure it is there before reading it. */
An HDLC hardware driver may call netif_stop_queue to temporarily stop the TX queue when the hardware is busy sending a frame, and after the hardware has finished sending the frame, call netif_wake_queue to resume the TX queue. However, the LAPB module doesn't know about this. Whether or not the hardware driver has stopped the TX queue, the LAPB module still feeds outgoing frames to the hardware driver for transmission. This can cause frames to be dropped by the hardware driver. It's not easy to fix this issue in the LAPB module. We can indeed let the LAPB module check whether the TX queue has been stopped before feeding each frame to the hardware driver, but when the hardware driver resumes the TX queue, it's not easy to immediately notify the LAPB module and ask it to resume transmission. Instead, we can fix this issue at the hdlc_x25 layer, by using qdisc TX queues to queue outgoing LAPB frames. The qdisc TX queue will then automatically be controlled by netif_stop_queue and netif_wake_queue. This way, when sending, we will use the qdisc queue to queue and send the data twice: once as the L3 packet and then (after processed by the LAPB module) as an LAPB (L2) frame. This does not make the logic of the code messy, because when receiving, data are already "received" on the device twice: once as an LAPB (L2) frame and then (after processed by the LAPB module) as the L3 packet. Some more details about the code change: 1. dev_queue_xmit_nit is removed because we already have it when we send the skb through the qdisc TX queue (in xmit_one). 2. hdlc_type_trans is replaced by assigning skb->dev and skb->protocol directly. skb_reset_mac_header in hdlc_type_trans is no longer necessary because it will be called in __dev_queue_xmit. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: Martin Schiller <ms@dev.tdt.de> Cc: Krzysztof Halasa <khc@pm.waw.pl> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- drivers/net/wan/hdlc_x25.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)