| Message ID | 20230104125724.3587015-1-laforge@osmocom.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: hdlc: Increase maximum HDLC MTU | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Jan 04, 2023 at 01:57:24PM +0100, Harald Welte wrote: > The FRF 1.2 specification clearly states: > > > A maximum frame relay information field size of 1600 octets shall be > > supported by the network and the user. > > The linux kernel hdlc/fr code has so far had a maximum MTU of 1500 > octets. This may have been sufficient to transport "regular" Ethernet > frames of MTU 1500 via frame relay net-devices, but there are other > use cases than Ethernet. > > One such use case is the 3GPP Gb interface (TS 48.014, 48.016, 48.018) > operated over Frame Relay. There is open source software [2] > implementing those interfaces by means of AF_PACKET sockets over > Linux kernel hdlcX devices. > > And before anyone asks: Even in 2023 there are real-world deployments of > those interfaces over Frame Relay in production use. > > This patch doesn't change the default hdlcX netdev MTU, but permits > userspace to configure a higher MTU, in those cases needed. I could be reading the code wrong, but: https://elixir.bootlin.com/linux/latest/source/drivers/net/wan/hdlc.c#L231 static void hdlc_setup_dev(struct net_device *dev) { /* Re-init all variables changed by HDLC protocol drivers, * including ether_setup() called from hdlc_raw_eth.c. */ dev->flags = IFF_POINTOPOINT | IFF_NOARP; dev->priv_flags = IFF_WAN_HDLC; dev->mtu = HDLC_MAX_MTU; dev->min_mtu = 68; dev->max_mtu = HDLC_MAX_MTU; dev->type = ARPHRD_RAWHDLC; dev->hard_header_len = 0; dev->needed_headroom = 0; dev->addr_len = 0; dev->header_ops = &hdlc_null_ops; } This change does appear to change dev->mtu, not just dev->max_mtu? So i'm not sure the commit message is correct? Andrew
Dear Andrew, On Wed, Jan 04, 2023 at 06:52:32PM +0100, Andrew Lunn wrote: > This change does appear to change dev->mtu, not just dev->max_mtu? > So i'm not sure the commit message is correct? Thanks for your feedback, which seems correct. We've been using this patch in production for more than a year, but our use cases are all using larger MTU so this might have been unnoticed. I'll investigate and likely we have to introduce a new HDLC_DEFAULT_MTU of 1500 to achieve backwards compatibility.
Harald, Andrew is right. The default setting makes sure the packets fit into regular Ethernet on the other side of the link (which is, or was, the most common situation). I guess the mtu could be set trivially to 1500 with the max being 1600 or 16k or whatever. Now there is a second thing, the HDLC_MAX_MRU (which is set to 1600). This is the (fixed) size of RX (and TX) memory buffers on certain old cards (some of whom are ISA and maybe even use 8-bit XT-BUS transfer mode). I guess it doesn't concern you directly, but the MTU on those cards must be kept at most at HDLC_MAX_MRU - max size of the headers (= 10 + 14 + 4 or so, maybe more) or the packets generated by the IP stack won't go out correctly. Harald Welte <laforge@osmocom.org> writes: > +/* FRF 1.2 states the information field should be 1600 bytes. So in case of > + * a 4-byte header of Q.922, this results in a MTU of 1604 bytes */ > +#define HDLC_MAX_MTU 1604 /* as required for FR network (e.g. > carrying GPRS-NS) */ I think the "FR information field" is the data portion, without 2-byte Q.922 address, and without the 2-byte frame check sequence, but including e.g. UI and NLPID. This means, in the simplest case of IPv4/v6, max MTU of 1598 bytes (by default), and less than that with 802.1q (8-byte "snap" DLCI header format + 14-byte bridged Ethernet header + 4 byte .1q header). This was never very straightforward. I think maybe we change HDLC_MAX_MRU from 1600 to 1602 (2 bytes for the Q.922 address and 1600 for the "FR information field"), this shouldn't break anything and would IMHO make the code compliant with the FRF 1.2. Then we drop the HDLC_MAX_MTU completely and use ETH_MAX_MTU (which is 0xFFFF) for dev->max_mtu instead. Devices using fixed buffer sizes should override this to, I guess, the limit - 10 - 14 - 4. For dev->mtu we could, by default, use ETH_DATA_LEN which is 1500 bytes. Also the assignments in fr_add_pvc() should be changed to account for the hdlcX master device parameters. What do you think?
Hi Krzysztof, thanks for your detailed analyiss. On Thu, Jan 05, 2023 at 08:06:19AM +0100, Krzysztof Hałasa wrote: > Andrew is right. The default setting makes sure the packets fit into > regular Ethernet on the other side of the link (which is, or was, the > most common situation). I guess the mtu could be set trivially to 1500 > with the max being 1600 or 16k or whatever. great. > Now there is a second thing, the HDLC_MAX_MRU (which is set to 1600). > This is the (fixed) size of RX (and TX) memory buffers on certain old > cards (some of whom are ISA and maybe even use 8-bit XT-BUS transfer > mode). I guess it doesn't concern you directly, but the MTU on those > cards must be kept at most at HDLC_MAX_MRU - max size of the headers > (= 10 + 14 + 4 or so, maybe more) or the packets generated by the IP > stack won't go out correctly. Understood. Indeed it is not my immediate concern, as I don't have any of those old ISA or even 8-bit XT-bus cards. If anyone had some spares of such retronetworking hardware, I'd be very interested. In the actual "present day" production deployments we're using either the Osmocom icE1usb (2-port E1 USB) or Digium TE820 (8-port E1 PCIe) with DAHDI as and CONFIG_DAHDI_NET, which then uses the kernel HDLC. > > +/* FRF 1.2 states the information field should be 1600 bytes. So in case of > > + * a 4-byte header of Q.922, this results in a MTU of 1604 bytes */ > > +#define HDLC_MAX_MTU 1604 /* as required for FR network (e.g. > > carrying GPRS-NS) */ > > I think the "FR information field" is the data portion, without 2-byte > Q.922 address, and without the 2-byte frame check sequence, but > including e.g. UI and NLPID. In my understanding of Q.922/Q.921, the information field does not include the control field or the address field. So in your example, the UI would be the control field. So we have a FR frame consisting of: * flag * address field: typically 2 octets, in theory also 3 or 4-octet formats * control field: 2 octets for I/S format, 1 octet for U format * FCS: 2 octets * flag > This means, in the simplest case of IPv4/v6, > max MTU of 1598 bytes (by default), and less than that with 802.1q > (8-byte "snap" DLCI header format + 14-byte bridged Ethernet header + > 4 byte .1q header). This was never very straightforward. As indicated, I unfortunately lack any in-depth experience with deployments of Ethernet or IP over Frame Relay or HDLC. > I think maybe we change HDLC_MAX_MRU from 1600 to 1602 (2 bytes for the > Q.922 address and 1600 for the "FR information field"), this shouldn't > break anything and would IMHO make the code compliant with the FRF 1.2. I would argue it should be at least 1604 (2 bytes address and 2 bytes control + 1600 byte information field. > Then we drop the HDLC_MAX_MTU completely and use ETH_MAX_MTU (which is > 0xFFFF) for dev->max_mtu instead. Devices using fixed buffer sizes > should override this to, I guess, the limit - 10 - 14 - 4. > For dev->mtu we could, by default, use ETH_DATA_LEN which is 1500 bytes. > Also the assignments in fr_add_pvc() should be changed to account for > the hdlcX master device parameters. > > What do you think? Sounds all good to me, though as stated I cannot really say much on the IP/Eth encapsuiation formats due to lack of experience with those. btw: In case you're wondering why in osmocom we are using "raw" hdlc netdev and implementing FR in userspace: That's because we typically need to implement the network side of FR towards an external user, and we in general need more control over the Q.933 layer. It's easier to do this in userspace, given that all of the PVC are handled by a single application anyway. Regards, Harald
diff --git a/include/uapi/linux/hdlc.h b/include/uapi/linux/hdlc.h index d89cb3ee7c70..f6b01e883cb5 100644 --- a/include/uapi/linux/hdlc.h +++ b/include/uapi/linux/hdlc.h @@ -13,7 +13,9 @@ #define _UAPI__HDLC_H -#define HDLC_MAX_MTU 1500 /* Ethernet 1500 bytes */ +/* FRF 1.2 states the information field should be 1600 bytes. So in case of + * a 4-byte header of Q.922, this results in a MTU of 1604 bytes */ +#define HDLC_MAX_MTU 1604 /* as required for FR network (e.g. carrying GPRS-NS) */ #if 0 #define HDLC_MAX_MRU (HDLC_MAX_MTU + 10 + 14 + 4) /* for ETH+VLAN over FR */ #else