| Message ID | 20200903000658.89944-1-xie.he.0141@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] drivers/net/wan/hdlc_fr: Add needed_headroom for PVC devices | expand |
On Wed, 2 Sep 2020 17:06:58 -0700 Xie He wrote: > PVC devices are virtual devices in this driver stacked on top of the > actual HDLC device. They are the devices normal users would use. > PVC devices have two types: normal PVC devices and Ethernet-emulating > PVC devices. > > When transmitting data with PVC devices, the ndo_start_xmit function > will prepend a header of 4 or 10 bytes. Currently this driver requests > this headroom to be reserved for normal PVC devices by setting their > hard_header_len to 10. However, this does not work when these devices > are used with AF_PACKET/RAW sockets. Also, this driver does not request > this headroom for Ethernet-emulating PVC devices (but deals with this > problem by reallocating the skb when needed, which is not optimal). > > This patch replaces hard_header_len with needed_headroom, and set > needed_headroom for Ethernet-emulating PVC devices, too. This makes > the driver to request headroom for all PVC devices in all cases. Since this is a tunnel protocol on top of HDLC interfaces, and hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if hdlc devices actually prepend 16 bytes of header, though. CC: Willem as he was reviewing your similar patch recently. > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > index 9acad651ea1f..12b35404cd8e 100644 > --- a/drivers/net/wan/hdlc_fr.c > +++ b/drivers/net/wan/hdlc_fr.c > @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev) > { > dev->type = ARPHRD_DLCI; > dev->flags = IFF_POINTOPOINT; > - dev->hard_header_len = 10; > + dev->hard_header_len = 0; Is there a need to set this to 0? Will it not be zero after allocation? > dev->addr_len = 2; > netif_keep_dst(dev); > } > @@ -1093,6 +1093,7 @@ static int fr_add_pvc(struct net_device *frad, unsigned int dlci, int type) > dev->mtu = HDLC_MAX_MTU; > dev->min_mtu = 68; > dev->max_mtu = HDLC_MAX_MTU; > + dev->needed_headroom = 10; > dev->priv_flags |= IFF_NO_QUEUE; > dev->ml_priv = pvc; >
Thank you for your email, Jakub! On Fri, Sep 4, 2020 at 3:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Since this is a tunnel protocol on top of HDLC interfaces, and > hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually > set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if > hdlc devices actually prepend 16 bytes of header, though. The HDLC device is not actually prepending any header when it is used with this driver. When the PVC device has prepended its header and handed over the skb to the HDLC device, the HDLC device just hands it over to the hardware driver for transmission without prepending any header. If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we can see there is no "header_ops" implemented in these two files and all "skb_push" happen in the PVC device in hdlc_fr.c. For this reason, I have previously submitted a patch to change the value of hard_header_len of the HDLC device from 16 to 0, because it is not actually used. See: 2b7bcd967a0f (drivers/net/wan/hdlc: Change the default of hard_header_len to 0) > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > > index 9acad651ea1f..12b35404cd8e 100644 > > --- a/drivers/net/wan/hdlc_fr.c > > +++ b/drivers/net/wan/hdlc_fr.c > > @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev) > > { > > dev->type = ARPHRD_DLCI; > > dev->flags = IFF_POINTOPOINT; > > - dev->hard_header_len = 10; > > + dev->hard_header_len = 0; > > Is there a need to set this to 0? Will it not be zero after allocation? Oh. I understand your point. Theoretically we don't need to set it to 0 because it already has the default value of 0. I'm setting it to 0 only because I want to tell future developers that this value is intentionally set to 0, and it is not carelessly missed out.
On Fri, Sep 4, 2020 at 6:28 PM Xie He <xie.he.0141@gmail.com> wrote: > > The HDLC device is not actually prepending any header when it is used > with this driver. When the PVC device has prepended its header and > handed over the skb to the HDLC device, the HDLC device just hands it > over to the hardware driver for transmission without prepending any > header. > > If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we > can see there is no "header_ops" implemented in these two files and > all "skb_push" happen in the PVC device in hdlc_fr.c. I want to provide a little more information about the flow after an HDLC device's ndo_start_xmit is called. An HDLC hardware driver's ndo_start_xmit is required to point to hdlc_start_xmit in hdlc.c. When a HDLC device receives a call to its ndo_start_xmit, hdlc_start_xmit will check if the protocol driver has provided a xmit function. If it has provided this function, hdlc_start_xmit will call it to start transmission. If it has not, hdlc_start_xmit will directly call the hardware driver's function to start transmission. This driver (hdlc_fr) has not provided a xmit function in its hdlc_proto struct, so hdlc_start_xmit will directly call the hardware driver's function to transmit. So no header will be prepended after ndo_start_xmit is called. There would not be any header prepended before ndo_start_xmit is called, either, because there is no header_ops implemented in either hdlc.c or hdlc_fr.c. On Fri, Sep 4, 2020 at 6:28 PM Xie He <xie.he.0141@gmail.com> wrote: > > Thank you for your email, Jakub! > > On Fri, Sep 4, 2020 at 3:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > Since this is a tunnel protocol on top of HDLC interfaces, and > > hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually > > set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if > > hdlc devices actually prepend 16 bytes of header, though. > > The HDLC device is not actually prepending any header when it is used > with this driver. When the PVC device has prepended its header and > handed over the skb to the HDLC device, the HDLC device just hands it > over to the hardware driver for transmission without prepending any > header. > > If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we > can see there is no "header_ops" implemented in these two files and > all "skb_push" happen in the PVC device in hdlc_fr.c. > > For this reason, I have previously submitted a patch to change the > value of hard_header_len of the HDLC device from 16 to 0, because it > is not actually used. > > See: > 2b7bcd967a0f (drivers/net/wan/hdlc: Change the default of hard_header_len to 0) > > > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > > > index 9acad651ea1f..12b35404cd8e 100644 > > > --- a/drivers/net/wan/hdlc_fr.c > > > +++ b/drivers/net/wan/hdlc_fr.c > > > @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev) > > > { > > > dev->type = ARPHRD_DLCI; > > > dev->flags = IFF_POINTOPOINT; > > > - dev->hard_header_len = 10; > > > + dev->hard_header_len = 0; > > > > Is there a need to set this to 0? Will it not be zero after allocation? > > Oh. I understand your point. Theoretically we don't need to set it to > 0 because it already has the default value of 0. I'm setting it to 0 > only because I want to tell future developers that this value is > intentionally set to 0, and it is not carelessly missed out.
On Fri, 4 Sep 2020 18:57:27 -0700 Xie He wrote: > On Fri, Sep 4, 2020 at 6:28 PM Xie He <xie.he.0141@gmail.com> wrote: > > > > The HDLC device is not actually prepending any header when it is used > > with this driver. When the PVC device has prepended its header and > > handed over the skb to the HDLC device, the HDLC device just hands it > > over to the hardware driver for transmission without prepending any > > header. > > > > If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we > > can see there is no "header_ops" implemented in these two files and > > all "skb_push" happen in the PVC device in hdlc_fr.c. > > I want to provide a little more information about the flow after an > HDLC device's ndo_start_xmit is called. > > An HDLC hardware driver's ndo_start_xmit is required to point to > hdlc_start_xmit in hdlc.c. When a HDLC device receives a call to its > ndo_start_xmit, hdlc_start_xmit will check if the protocol driver has > provided a xmit function. If it has provided this function, > hdlc_start_xmit will call it to start transmission. If it has not, > hdlc_start_xmit will directly call the hardware driver's function to > start transmission. This driver (hdlc_fr) has not provided a xmit > function in its hdlc_proto struct, so hdlc_start_xmit will directly > call the hardware driver's function to transmit. > > So no header will be prepended after ndo_start_xmit is called. > > There would not be any header prepended before ndo_start_xmit is > called, either, because there is no header_ops implemented in either > hdlc.c or hdlc_fr.c. Thank you for the detailed explanation. > On Fri, Sep 4, 2020 at 6:28 PM Xie He <xie.he.0141@gmail.com> wrote: > > > > Thank you for your email, Jakub! > > > > On Fri, Sep 4, 2020 at 3:14 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > Since this is a tunnel protocol on top of HDLC interfaces, and > > > hdlc_setup_dev() sets dev->hard_header_len = 16; should we actually > > > set the needed_headroom to 10 + 16 = 26? I'm not clear on where/if > > > hdlc devices actually prepend 16 bytes of header, though. > > > > The HDLC device is not actually prepending any header when it is used > > with this driver. When the PVC device has prepended its header and > > handed over the skb to the HDLC device, the HDLC device just hands it > > over to the hardware driver for transmission without prepending any > > header. > > > > If we grep "header_ops" and "skb_push" in "hdlc.c" and "hdlc_fr.c", we > > can see there is no "header_ops" implemented in these two files and > > all "skb_push" happen in the PVC device in hdlc_fr.c. > > > > For this reason, I have previously submitted a patch to change the > > value of hard_header_len of the HDLC device from 16 to 0, because it > > is not actually used. > > > > See: > > 2b7bcd967a0f (drivers/net/wan/hdlc: Change the default of hard_header_len to 0) Ah, sorry.. the tree I was looking at did not have this commit. > > > > diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c > > > > index 9acad651ea1f..12b35404cd8e 100644 > > > > --- a/drivers/net/wan/hdlc_fr.c > > > > +++ b/drivers/net/wan/hdlc_fr.c > > > > @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev) > > > > { > > > > dev->type = ARPHRD_DLCI; > > > > dev->flags = IFF_POINTOPOINT; > > > > - dev->hard_header_len = 10; > > > > + dev->hard_header_len = 0; > > > > > > Is there a need to set this to 0? Will it not be zero after allocation? > > > > Oh. I understand your point. Theoretically we don't need to set it to > > 0 because it already has the default value of 0. I'm setting it to 0 > > only because I want to tell future developers that this value is > > intentionally set to 0, and it is not carelessly missed out. Sounds fair. Applied to net, thank you!
On Fri, Sep 4, 2020 at 9:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Applied to net, thank you! Thank you, Jakub!
Xie He <xie.he.0141@gmail.com> writes: > The HDLC device is not actually prepending any header when it is used > with this driver. When the PVC device has prepended its header and > handed over the skb to the HDLC device, the HDLC device just hands it > over to the hardware driver for transmission without prepending any > header. That's correct. IIRC: - Cisco and PPP modes add 4 bytes - Frame Relay adds 4 (specific protocols - mostly IPv4) or 10 (general case) bytes. There is that pvcX->hdlcX transition which adds nothing (the header is already in place when the packet leaves pvcX device). - Raw mode adds nothing (IPv4 only, though it could be modified for both IPv4/v6 easily) - Ethernet (hdlc_raw_eth.c) adds normal Ethernet header. (I had been "unplugged" for some time).
On Sun, Sep 13, 2020 at 10:26 PM Krzysztof Hałasa <khalasa@piap.pl> wrote: > > Xie He <xie.he.0141@gmail.com> writes: > > > The HDLC device is not actually prepending any header when it is used > > with this driver. When the PVC device has prepended its header and > > handed over the skb to the HDLC device, the HDLC device just hands it > > over to the hardware driver for transmission without prepending any > > header. > > That's correct. IIRC: > - Cisco and PPP modes add 4 bytes > - Frame Relay adds 4 (specific protocols - mostly IPv4) or 10 (general > case) bytes. There is that pvcX->hdlcX transition which adds nothing > (the header is already in place when the packet leaves pvcX device). > - Raw mode adds nothing (IPv4 only, though it could be modified for > both IPv4/v6 easily) > - Ethernet (hdlc_raw_eth.c) adds normal Ethernet header. Thank you for the nice summary! I just realized that we can indeed support both IPv4/v6 in hdlc_raw. Both IPv4 and v6 packets have a version field in the header, so we can use it to distinguish v4 and v6 packets on receiving. We can introduce it as a new feature some time.
diff --git a/drivers/net/wan/hdlc_fr.c b/drivers/net/wan/hdlc_fr.c index 9acad651ea1f..12b35404cd8e 100644 --- a/drivers/net/wan/hdlc_fr.c +++ b/drivers/net/wan/hdlc_fr.c @@ -1041,7 +1041,7 @@ static void pvc_setup(struct net_device *dev) { dev->type = ARPHRD_DLCI; dev->flags = IFF_POINTOPOINT; - dev->hard_header_len = 10; + dev->hard_header_len = 0; dev->addr_len = 2; netif_keep_dst(dev); } @@ -1093,6 +1093,7 @@ static int fr_add_pvc(struct net_device *frad, unsigned int dlci, int type) dev->mtu = HDLC_MAX_MTU; dev->min_mtu = 68; dev->max_mtu = HDLC_MAX_MTU; + dev->needed_headroom = 10; dev->priv_flags |= IFF_NO_QUEUE; dev->ml_priv = pvc;
PVC devices are virtual devices in this driver stacked on top of the actual HDLC device. They are the devices normal users would use. PVC devices have two types: normal PVC devices and Ethernet-emulating PVC devices. When transmitting data with PVC devices, the ndo_start_xmit function will prepend a header of 4 or 10 bytes. Currently this driver requests this headroom to be reserved for normal PVC devices by setting their hard_header_len to 10. However, this does not work when these devices are used with AF_PACKET/RAW sockets. Also, this driver does not request this headroom for Ethernet-emulating PVC devices (but deals with this problem by reallocating the skb when needed, which is not optimal). This patch replaces hard_header_len with needed_headroom, and set needed_headroom for Ethernet-emulating PVC devices, too. This makes the driver to request headroom for all PVC devices in all cases. Cc: Krzysztof Halasa <khc@pm.waw.pl> Cc: Martin Schiller <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- Change from v1: English language fix for the commit message. Changed "Ethernet-emulated" to "Ethernet-emulating" because the device is emulating an Ethernet device, rather than being emulated by an Ethernet device. I'm sorry for my poor English. --- drivers/net/wan/hdlc_fr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)