Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 11:30:25PM -0800, Xie He wrote: > On Mon, Feb 15, 2021 at 10:04 PM Leon Romanovsky wrote: > > > > On Mon, Feb 15, 2021 at 11:08:02AM -0800, Xie He wrote: > > > On Mon, Feb 15, 2021 at 10:54 AM Leon Romanovsky wrote: > > > > > > > > On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > > > > > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky > > > > > wrote: > > > > > > > > > > > > > + /* 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. > > > > > > > + */ > > > > > > > + dev->needed_headroom = 3 - 1; > > > > > > > > > > > > 3 - 1 = 2 > > > > > > > > > > > > Thanks > > > > > > > > > > Actually this is intentional. It makes the numbers more meaningful. > > > > > > > > > > The compiler should automatically generate the "2" so there would be > > > > > no runtime penalty. > > > > > > > > If you want it intentional, write it in the comment. > > > > > > > > /* When transmitting data, we will need extra 2 bytes headroom, > > > > * which are 3 bytes of LAPB header minus one byte of pseudo header. > > > > */ > > > > dev->needed_headroom = 2; > > > > > > I think this is unnecessary. The current comment already explains the > > > meaning of the "1" and the "3". There's no need for a reader of this > > > code to understand what a "2" is. That is the job of the compiler, not > > > the human reader. > > > > It is not related to compiler/human format. If you need to write "3 - 1" > > to make it easy for users, it means that your comment above is not > > full/correct/e.t.c. > > My point is: there is no need for human programmers / code readers to > understand what this "2" is. There is no need to explain what this "2" > means in the comment. There is no need to write this "2" in the code. > There is no need for this "2" to appear anywhere. That is just an > intermediate result generated by the compiler. It is similar to > assembly or machine code. It is generated by the compiler. Human > programmers just don't care about this intermediate result. > > My point could be more apparent if you consider a more complex > situation: "3 - 1 + 2 + 4 + 2". No human would want to see a > meaningless "10" in the code. We want to see the meaning of the > numbers, not a meaningless intermediate calculation result. Right, and we are using defines for that. Thanks
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 10:04 PM Leon Romanovsky wrote: > > On Mon, Feb 15, 2021 at 11:08:02AM -0800, Xie He wrote: > > On Mon, Feb 15, 2021 at 10:54 AM Leon Romanovsky wrote: > > > > > > On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > > > > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > > > > > > > > > + /* 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. > > > > > > + */ > > > > > > + dev->needed_headroom = 3 - 1; > > > > > > > > > > 3 - 1 = 2 > > > > > > > > > > Thanks > > > > > > > > Actually this is intentional. It makes the numbers more meaningful. > > > > > > > > The compiler should automatically generate the "2" so there would be > > > > no runtime penalty. > > > > > > If you want it intentional, write it in the comment. > > > > > > /* When transmitting data, we will need extra 2 bytes headroom, > > > * which are 3 bytes of LAPB header minus one byte of pseudo header. > > > */ > > > dev->needed_headroom = 2; > > > > I think this is unnecessary. The current comment already explains the > > meaning of the "1" and the "3". There's no need for a reader of this > > code to understand what a "2" is. That is the job of the compiler, not > > the human reader. > > It is not related to compiler/human format. If you need to write "3 - 1" > to make it easy for users, it means that your comment above is not > full/correct/e.t.c. My point is: there is no need for human programmers / code readers to understand what this "2" is. There is no need to explain what this "2" means in the comment. There is no need to write this "2" in the code. There is no need for this "2" to appear anywhere. That is just an intermediate result generated by the compiler. It is similar to assembly or machine code. It is generated by the compiler. Human programmers just don't care about this intermediate result. My point could be more apparent if you consider a more complex situation: "3 - 1 + 2 + 4 + 2". No human would want to see a meaningless "10" in the code. We want to see the meaning of the numbers, not a meaningless intermediate calculation result.
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 11:08:02AM -0800, Xie He wrote: > On Mon, Feb 15, 2021 at 10:54 AM Leon Romanovsky wrote: > > > > On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > > > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > > > > > > > + /* 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. > > > > > + */ > > > > > + dev->needed_headroom = 3 - 1; > > > > > > > > 3 - 1 = 2 > > > > > > > > Thanks > > > > > > Actually this is intentional. It makes the numbers more meaningful. > > > > > > The compiler should automatically generate the "2" so there would be > > > no runtime penalty. > > > > If you want it intentional, write it in the comment. > > > > /* When transmitting data, we will need extra 2 bytes headroom, > > * which are 3 bytes of LAPB header minus one byte of pseudo header. > > */ > > dev->needed_headroom = 2; > > I think this is unnecessary. The current comment already explains the > meaning of the "1" and the "3". There's no need for a reader of this > code to understand what a "2" is. That is the job of the compiler, not > the human reader. It is not related to compiler/human format. If you need to write "3 - 1" to make it easy for users, it means that your comment above is not full/correct/e.t.c. Thanks
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 10:54 AM Leon Romanovsky wrote: > > On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > > > > > + /* 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. > > > > + */ > > > > + dev->needed_headroom = 3 - 1; > > > > > > 3 - 1 = 2 > > > > > > Thanks > > > > Actually this is intentional. It makes the numbers more meaningful. > > > > The compiler should automatically generate the "2" so there would be > > no runtime penalty. > > If you want it intentional, write it in the comment. > > /* When transmitting data, we will need extra 2 bytes headroom, > * which are 3 bytes of LAPB header minus one byte of pseudo header. > */ > dev->needed_headroom = 2; I think this is unnecessary. The current comment already explains the meaning of the "1" and the "3". There's no need for a reader of this code to understand what a "2" is. That is the job of the compiler, not the human reader.
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 09:23:32AM -0800, Xie He wrote: > On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > > > + /* 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. > > > + */ > > > + dev->needed_headroom = 3 - 1; > > > > 3 - 1 = 2 > > > > Thanks > > Actually this is intentional. It makes the numbers more meaningful. > > The compiler should automatically generate the "2" so there would be > no runtime penalty. If you want it intentional, write it in the comment. /* When transmitting data, we will need extra 2 bytes headroom, * which are 3 bytes of LAPB header minus one byte of pseudo header. */ dev->needed_headroom = 2;
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Mon, Feb 15, 2021 at 1:25 AM Leon Romanovsky wrote: > > > + /* 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. > > + */ > > + dev->needed_headroom = 3 - 1; > > 3 - 1 = 2 > > Thanks Actually this is intentional. It makes the numbers more meaningful. The compiler should automatically generate the "2" so there would be no runtime penalty.
Re: [PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
On Sun, Feb 14, 2021 at 11:27:03PM -0800, Xie He wrote:
> When sending packets, we will first hand over the (L3) packets to the
> LAPB module. The LAPB module will then 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, even without
> an (L3) packet currently being sent on the device. This happens when the
> LAPB module tries to send (L3) packets queued up in its internal queue,
> or when the LAPB module decides to send some (L2) control frame.
>
> This means we need to have a queue for these outgoing LAPB (L2) frames,
> otherwise frames can be dropped if sent when the hardware driver is
> already busy in transmitting. The queue needs to be controlled by
> the hardware driver's netif_stop_queue and netif_wake_queue calls.
> Therefore, we need to use the device's qdisc TX queue for this purpose.
> However, currently outgoing LAPB (L2) frames are not queued.
>
> On the other hand, outgoing (L3) packets (before they are handed over
> to the LAPB module) don't need to be queued, because the LAPB module
> already has an internal queue for them, and is able to queue new outgoing
> (L3) packets at any time. However, currently outgoing (L3) packets are
> being queued in the device's qdisc TX queue, which is controlled by
> the hardware driver's netif_stop_queue and netif_wake_queue calls.
> This is unnecessary and meaningless.
>
> To fix these issues, we can split the HDLC device into two devices -
> a virtual X.25 device and the actual HDLC device, use the virtual X.25
> device to send (L3) packets and then use the actual HDLC device to
> queue LAPB (L2) frames. The outgoing (L2) LAPB queue will be controlled
> by the hardware driver's netif_stop_queue and netif_wake_queue calls,
> while outgoing (L3) packets will not be affected by these calls.
>
> Cc: Martin Schiller
> Signed-off-by: Xie He
> ---
>
> Change from RFC v2:
> Simplified the commit message.
> Dropped the x25_open fix which is already merged into net-next now.
> Use HDLC_MAX_MTU as the mtu of the X.25 virtual device.
> Add an explanation to the documentation about the X.25 virtual device.
>
> Change from RFC v1:
> Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
>
> ---
> Documentation/networking/generic-hdlc.rst | 3 +
> drivers/net/wan/hdlc_x25.c| 153 ++
> 2 files changed, 130 insertions(+), 26 deletions(-)
<...>
> +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;
> + dev->mtu = HDLC_MAX_MTU;
> +
> + /* 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.
> + */
> + dev->needed_headroom = 3 - 1;
3 - 1 = 2
Thanks
> +}
> +
[PATCH net-next RFC v3] net: hdlc_x25: Queue outgoing LAPB frames
When sending packets, we will first hand over the (L3) packets to the
LAPB module. The LAPB module will then 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, even without
an (L3) packet currently being sent on the device. This happens when the
LAPB module tries to send (L3) packets queued up in its internal queue,
or when the LAPB module decides to send some (L2) control frame.
This means we need to have a queue for these outgoing LAPB (L2) frames,
otherwise frames can be dropped if sent when the hardware driver is
already busy in transmitting. The queue needs to be controlled by
the hardware driver's netif_stop_queue and netif_wake_queue calls.
Therefore, we need to use the device's qdisc TX queue for this purpose.
However, currently outgoing LAPB (L2) frames are not queued.
On the other hand, outgoing (L3) packets (before they are handed over
to the LAPB module) don't need to be queued, because the LAPB module
already has an internal queue for them, and is able to queue new outgoing
(L3) packets at any time. However, currently outgoing (L3) packets are
being queued in the device's qdisc TX queue, which is controlled by
the hardware driver's netif_stop_queue and netif_wake_queue calls.
This is unnecessary and meaningless.
To fix these issues, we can split the HDLC device into two devices -
a virtual X.25 device and the actual HDLC device, use the virtual X.25
device to send (L3) packets and then use the actual HDLC device to
queue LAPB (L2) frames. The outgoing (L2) LAPB queue will be controlled
by the hardware driver's netif_stop_queue and netif_wake_queue calls,
while outgoing (L3) packets will not be affected by these calls.
Cc: Martin Schiller
Signed-off-by: Xie He
---
Change from RFC v2:
Simplified the commit message.
Dropped the x25_open fix which is already merged into net-next now.
Use HDLC_MAX_MTU as the mtu of the X.25 virtual device.
Add an explanation to the documentation about the X.25 virtual device.
Change from RFC v1:
Properly initialize state(hdlc)->x25_dev and state(hdlc)->x25_dev_lock.
---
Documentation/networking/generic-hdlc.rst | 3 +
drivers/net/wan/hdlc_x25.c| 153 ++
2 files changed, 130 insertions(+), 26 deletions(-)
diff --git a/Documentation/networking/generic-hdlc.rst
b/Documentation/networking/generic-hdlc.rst
index 1c3bb5cb98d4..55f6b0ab45be 100644
--- a/Documentation/networking/generic-hdlc.rst
+++ b/Documentation/networking/generic-hdlc.rst
@@ -59,6 +59,9 @@ or::
In Frame Relay mode, ifconfig master hdlc device up (without assigning
any IP address to it) before using pvc devices.
+In X.25 mode, ifconfig the hdlc device up, then a virtual X.25 device
+would appear for use.
+
Setting interface:
diff --git a/drivers/net/wan/hdlc_x25.c b/drivers/net/wan/hdlc_x25.c
index 4aaa6388b9ee..b0541f1bfb4e 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,7 +170,8 @@ 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;
@@ -195,9 +203,98 @@ static int x25_open(struct net_device *dev)
-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 =
