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 <[email protected]>
Cc: Krzysztof Halasa <[email protected]>
Signed-off-by: Xie He <[email protected]>
---
drivers/net/wan/hdlc_x25.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
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.
*/
--
2.27.0
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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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.