On 2021-01-31 04:16, Xie He wrote:
On Sat, Jan 30, 2021 at 11:16 AM Jakub Kicinski <k...@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?
