| Message ID | 20210216201813.60394-1-xie.he.0141@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,RFC,v4] net: hdlc_x25: Queue outgoing LAPB frames | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 225 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Feb 16, 2021 at 12:18:13PM -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 <ms@dev.tdt.de> > Signed-off-by: Xie He <xie.he.0141@gmail.com> > --- <...> > +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; It is nice that you are resending your patch without the resolution. However it will be awesome if you don't ignore review comments and fix this "3 - 1" by writing solid comment above. Thanks and good luck.
On Thu, Feb 18, 2021 at 12:57 AM Leon Romanovsky <leon@kernel.org> wrote: > > It is nice that you are resending your patch without the resolution. > However it will be awesome if you don't ignore review comments and fix this "3 - 1" > by writing solid comment above. I thought you already agreed with me? It looks like you didn't? I still don't think there is any problem with my current way. I still don't understand your point. What problem do you think is there? Why is your way better than my way? I've already given multiple reasons about why my way is better than yours. But you didn't explain clearly why yours is better than mine.
On Thu, Feb 18, 2021 at 01:07:13AM -0800, Xie He wrote: > On Thu, Feb 18, 2021 at 12:57 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > It is nice that you are resending your patch without the resolution. > > However it will be awesome if you don't ignore review comments and fix this "3 - 1" > > by writing solid comment above. > > I thought you already agreed with me? It looks like you didn't? > > I still don't think there is any problem with my current way. > > I still don't understand your point. What problem do you think is > there? Why is your way better than my way? I've already given multiple > reasons about why my way is better than yours. But you didn't explain > clearly why yours is better than mine. It is not me who didn't explain, it is you who didn't want to write clear comment that describes the headroom size without need of "3 - 1". So in current situation, you added two things: comment and assignment. Both of them aren't serve their goals. Your comment doesn't explain enough and needs extra help and your assignment is useless without comment. Thanks
On Thu, Feb 18, 2021 at 2:37 AM Leon Romanovsky <leon@kernel.org> wrote: > > It is not me who didn't explain, it is you who didn't want to write clear > comment that describes the headroom size without need of "3 - 1". Why do I need to write unnecessary comments when "3 - 1" and the current comment already explains everything? > So in current situation, you added two things: comment and assignment. > Both of them aren't serve their goals. Why? > Your comment doesn't explain > enough and needs extra help Why? My comment already explains everything. > and your assignment is useless without > comment. My assignment is already very clear with my current comment. My comment explains very clearly what this assignment means.
On Thu, Feb 18, 2021 at 09:36:54AM -0800, Xie He wrote: > On Thu, Feb 18, 2021 at 2:37 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > It is not me who didn't explain, it is you who didn't want to write clear > > comment that describes the headroom size without need of "3 - 1". > > Why do I need to write unnecessary comments when "3 - 1" and the > current comment already explains everything? This is how we write code, we use defines instead of constant numbers, comments to describe tricky parts and assign already preprocessed result. There is nothing I can do If you don't like or don't want to use Linux kernel style. Thanks
On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky <leon@kernel.org> wrote: > > This is how we write code, we use defines instead of constant numbers, > comments to describe tricky parts and assign already preprocessed result. > > There is nothing I can do If you don't like or don't want to use Linux kernel > style. So what is your suggestion exactly? Use defines or write comments? As I understand, you want to replace the "3 - 1" with "2", and then write comments to explain that this "2" is the result of "3 - 1". Why do you want to do this? You are doing useless things and you force readers of this code to think about useless things. You said this was "Linux kernel style"? Why? Which sentence of the Linux kernel style guide suggests your way is better than my way?
On Thu, Feb 18, 2021 at 12:06 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > This is how we write code, we use defines instead of constant numbers, > > comments to describe tricky parts and assign already preprocessed result. > > > > There is nothing I can do If you don't like or don't want to use Linux kernel > > style. > > So what is your suggestion exactly? Use defines or write comments? > > As I understand, you want to replace the "3 - 1" with "2", and then > write comments to explain that this "2" is the result of "3 - 1". > > Why do you want to do this? You are doing useless things and you force > readers of this code to think about useless things. > > You said this was "Linux kernel style"? Why? Which sentence of the > Linux kernel style guide suggests your way is better than my way? Nevermind, if you *really* want me to replace this "3 - 1" with "2" and explain in the comment that the "2" is a result of "3 - 1". I'll do this. I admit this is a style issue. So it is hard to argue and reach an agreement. Just reply with a request and I'll make the change. However I'm not able to agree with you in my heart. On Thu, Feb 18, 2021 at 12:06 PM Xie He <xie.he.0141@gmail.com> wrote: > > On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > This is how we write code, we use defines instead of constant numbers, > > comments to describe tricky parts and assign already preprocessed result. > > > > There is nothing I can do If you don't like or don't want to use Linux kernel > > style. > > So what is your suggestion exactly? Use defines or write comments? > > As I understand, you want to replace the "3 - 1" with "2", and then > write comments to explain that this "2" is the result of "3 - 1". > > Why do you want to do this? You are doing useless things and you force > readers of this code to think about useless things. > > You said this was "Linux kernel style"? Why? Which sentence of the > Linux kernel style guide suggests your way is better than my way?
On Thu, 18 Feb 2021 12:23:28 -0800 Xie He wrote: > On Thu, Feb 18, 2021 at 12:06 PM Xie He <xie.he.0141@gmail.com> wrote: > > > > On Thu, Feb 18, 2021 at 11:55 AM Leon Romanovsky <leon@kernel.org> wrote: > > > > > > This is how we write code, we use defines instead of constant numbers, > > > comments to describe tricky parts and assign already preprocessed result. > > > > > > There is nothing I can do If you don't like or don't want to use Linux kernel > > > style. > > > > So what is your suggestion exactly? Use defines or write comments? > > > > As I understand, you want to replace the "3 - 1" with "2", and then > > write comments to explain that this "2" is the result of "3 - 1". > > > > Why do you want to do this? You are doing useless things and you force > > readers of this code to think about useless things. > > > > You said this was "Linux kernel style"? Why? Which sentence of the > > Linux kernel style guide suggests your way is better than my way? > > Nevermind, if you *really* want me to replace this "3 - 1" with "2" > and explain in the comment that the "2" is a result of "3 - 1". I'll > do this. I admit this is a style issue. So it is hard to argue and > reach an agreement. Just reply with a request and I'll make the > change. However I'm not able to agree with you in my heart. Not entirely sure what the argument is about but adding constants would certainly help. More fundamentally IDK if we can make such a fundamental change here. When users upgrade from older kernel are all their scripts going to work the same? Won't they have to bring the new netdev up?
On Fri, Feb 19, 2021 at 10:39 AM Jakub Kicinski <kuba@kernel.org> wrote: > > Not entirely sure what the argument is about but adding constants would > certainly help. Leon wants me to replace this: dev->needed_headroom = 3 - 1; with this: /* 2 is the result of 3 - 1 */ dev->needed_headroom = 2; But I don't feel his way is better than my way. > More fundamentally IDK if we can make such a fundamental change here. > When users upgrade from older kernel are all their scripts going to > work the same? Won't they have to bring the new netdev up? Yes, this patch will break backward compatibility. Users with old scripts will find them no longer working. However, it's hard for me to find a better way to solve the problem described in the commit message. So I sent this as an RFC to see what people think about this. (Martin Schiller seems to be OK with this.) I think users who don't use scripts can adapt quickly and users who use scripts can also trivally fix their scripts. Actually many existing commits in the kernel also (more or less) cause some user-visible changes. But I admit this patch is a really big change.
On Fri, Feb 19, 2021 at 12:28:12PM -0800, Xie He wrote: > On Fri, Feb 19, 2021 at 10:39 AM Jakub Kicinski <kuba@kernel.org> wrote: > > > > Not entirely sure what the argument is about but adding constants would > > certainly help. > > Leon wants me to replace this: > > dev->needed_headroom = 3 - 1; > > with this: > Leon wants this line to be written good enough: > /* 2 is the result of 3 - 1 */ And this line like you wrote here: > dev->needed_headroom = 2; <...> > Yes, this patch will break backward compatibility. Users with old > scripts will find them no longer working. Did you search in debian/fedora code repositories to see if such scripts exist as part of any distro package? Thanks
On Sat, Feb 20, 2021 at 10:39 PM Leon Romanovsky <leon@kernel.org> wrote: > > > Yes, this patch will break backward compatibility. Users with old > > scripts will find them no longer working. > > Did you search in debian/fedora code repositories to see if such scripts exist > as part of any distro package? I just tried to search in Debian and Fedora packages but didn't seem to find anything related. I searched "hdlc" and "x25". The only things I can find are "AX.25" related things. But "AX.25" is not related to "X.25". I guess a script that brings a network interface up might not be very useful? So we might not be able to find it in public repositories.
On 2021-02-19 21:28, Xie He wrote: > On Fri, Feb 19, 2021 at 10:39 AM Jakub Kicinski <kuba@kernel.org> > wrote: >> >> Not entirely sure what the argument is about but adding constants >> would >> certainly help. > > Leon wants me to replace this: > > dev->needed_headroom = 3 - 1; > > with this: > > /* 2 is the result of 3 - 1 */ > dev->needed_headroom = 2; > > But I don't feel his way is better than my way. > >> More fundamentally IDK if we can make such a fundamental change here. >> When users upgrade from older kernel are all their scripts going to >> work the same? Won't they have to bring the new netdev up? > > Yes, this patch will break backward compatibility. Users with old > scripts will find them no longer working. > > However, it's hard for me to find a better way to solve the problem > described in the commit message. > > So I sent this as an RFC to see what people think about this. (Martin > Schiller seems to be OK with this.) Well, I said I would like to test it. I'm not really happy with this change because it breaks compatibility. We then suddenly have 2 interfaces; the X.25 routings are to be set via the "new" hdlc<x>_x25 interfaces instead of the hdlc<x> interfaces. I currently just don't have a nicer solution to fix this queueing problem either. On the other hand, since the many years we have been using the current state, I have never noticed any problems with discarded frames. So it might be more a theoretical problem than a practical one. > > I think users who don't use scripts can adapt quickly and users who > use scripts can also trivally fix their scripts. > > Actually many existing commits in the kernel also (more or less) cause > some user-visible changes. But I admit this patch is a really big > change.
On Sun, Feb 21, 2021 at 11:14 PM Martin Schiller <ms@dev.tdt.de> wrote: > > I'm not really happy with this change because it breaks compatibility. > We then suddenly have 2 interfaces; the X.25 routings are to be set via > the "new" hdlc<x>_x25 interfaces instead of the hdlc<x> interfaces. > > I currently just don't have a nicer solution to fix this queueing > problem either. On the other hand, since the many years we have been > using the current state, I have never noticed any problems with > discarded frames. So it might be more a theoretical problem than a > practical one. This problem becomes very serious when we use AF_PACKET sockets, because the majority of frames would be dropped by the hardware driver, which significantly impacts transmission speed. What I am really doing is to enable adequate support for AF_PACKET sockets, allowing users to use the bare (raw) LAPB protocol. If we take this into consideration, this problem is no longer just a theoretical problem, but a real practical issue. If we don't want to break backward compatibility, there is another option: We can create a new API for the HDLC subsystem for stopping/restarting the TX queue, and replace all HDLC hardware drivers' netif_stop_queue and netif_wake_queue calls with calls to this new API. This new API would then call hdlc_x25 to stop/restart its internal queue. But this option would require modifying all HDLC hardware drivers' code, and frankly, not all HDLC hardware drivers' developers care about running X.25 protocols on their hardware. So this would cause both hardware driver instabilities and confusion for hardware driver developers.
On 2021-02-22 09:56, Xie He wrote: > On Sun, Feb 21, 2021 at 11:14 PM Martin Schiller <ms@dev.tdt.de> wrote: >> >> I'm not really happy with this change because it breaks compatibility. >> We then suddenly have 2 interfaces; the X.25 routings are to be set >> via >> the "new" hdlc<x>_x25 interfaces instead of the hdlc<x> interfaces. >> >> I currently just don't have a nicer solution to fix this queueing >> problem either. On the other hand, since the many years we have been >> using the current state, I have never noticed any problems with >> discarded frames. So it might be more a theoretical problem than a >> practical one. > > This problem becomes very serious when we use AF_PACKET sockets, > because the majority of frames would be dropped by the hardware > driver, which significantly impacts transmission speed. What I am > really doing is to enable adequate support for AF_PACKET sockets, > allowing users to use the bare (raw) LAPB protocol. If we take this > into consideration, this problem is no longer just a theoretical > problem, but a real practical issue. I have now had a look at it. It works as expected. I just wonder if it would not be more appropriate to call the lapb_register() already in x25_hdlc_open(), so that the layer2 (lapb) can already "work" before the hdlc<x>_x25 interface is up. Also, I have a hard time assessing if such a wrap is really enforceable. Unfortunately I have no idea how many users there actually are. > > If we don't want to break backward compatibility, there is another > option: > We can create a new API for the HDLC subsystem for stopping/restarting > the TX queue, and replace all HDLC hardware drivers' netif_stop_queue > and netif_wake_queue calls with calls to this new API. This new API > would then call hdlc_x25 to stop/restart its internal queue. > > But this option would require modifying all HDLC hardware drivers' > code, and frankly, not all HDLC hardware drivers' developers care > about running X.25 protocols on their hardware. So this would cause > both hardware driver instabilities and confusion for hardware driver > developers.
On Fri, Feb 26, 2021 at 6:21 AM Martin Schiller <ms@dev.tdt.de> wrote: > > I have now had a look at it. It works as expected. > I just wonder if it would not be more appropriate to call > the lapb_register() already in x25_hdlc_open(), so that the layer2 > (lapb) can already "work" before the hdlc<x>_x25 interface is up. I think it's better not to keep LAPB running unless hdlc<x>_x25 is up. If I am the user, I would expect that when I change the X.25 interface to the DOWN state, the LAPB protocol would be completely stopped and the LAPB layer would not generate any new frames anymore (even if the other side wants to connect), and when I change the X.25 interface back to the UP state, it would be a fresh new start for the LAPB protocol. > Also, I have a hard time assessing if such a wrap is really enforceable. Sorry. I don't understand what you mean. What "wrap" are you referring to? > Unfortunately I have no idea how many users there actually are.
On 2021-02-27 00:03, Xie He wrote: > On Fri, Feb 26, 2021 at 6:21 AM Martin Schiller <ms@dev.tdt.de> wrote: >> >> I have now had a look at it. It works as expected. >> I just wonder if it would not be more appropriate to call >> the lapb_register() already in x25_hdlc_open(), so that the layer2 >> (lapb) can already "work" before the hdlc<x>_x25 interface is up. > > I think it's better not to keep LAPB running unless hdlc<x>_x25 is up. > If I am the user, I would expect that when I change the X.25 interface > to the DOWN state, the LAPB protocol would be completely stopped and > the LAPB layer would not generate any new frames anymore (even if the > other side wants to connect), and when I change the X.25 interface > back to the UP state, it would be a fresh new start for the LAPB > protocol. > >> Also, I have a hard time assessing if such a wrap is really >> enforceable. > > Sorry. I don't understand what you mean. What "wrap" are you referring > to? I mean the change from only one hdlc<x> interface to both hdlc<x> and hdlc<x>_x25. I can't estimate how many users are out there and how their setup looks like. > >> Unfortunately I have no idea how many users there actually are.
On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller <ms@dev.tdt.de> wrote: > > >> Also, I have a hard time assessing if such a wrap is really > >> enforceable. > > > > Sorry. I don't understand what you mean. What "wrap" are you referring > > to? > > I mean the change from only one hdlc<x> interface to both hdlc<x> and > hdlc<x>_x25. > > I can't estimate how many users are out there and how their setup looks > like. I'm also thinking about solving this issue by adding new APIs to the HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware drivers to call instead of netif_stop_queue / netif_wake_queue. This way we can preserve backward compatibility. However I'm reluctant to change the code of all the hardware drivers because I'm afraid of introducing bugs, etc. When I look at the code of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no bugs (related to stop_queue / wake_queue) after my change (and even before my change, actually). There are even serious style problems: the majority of its lines are indented by spaces. So I don't want to mess with all the hardware drivers. Hardware driver developers (if they wish to properly support hdlc_x25) should do the change themselves. This is not a problem for me, because I use my own out-of-tree hardware driver. However if I add APIs with no user code in the kernel, other developers may think these APIs are not necessary.
On 2021-03-01 09:56, Xie He wrote: > On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller <ms@dev.tdt.de> wrote: >> >> >> Also, I have a hard time assessing if such a wrap is really >> >> enforceable. >> > >> > Sorry. I don't understand what you mean. What "wrap" are you referring >> > to? >> >> I mean the change from only one hdlc<x> interface to both hdlc<x> and >> hdlc<x>_x25. >> >> I can't estimate how many users are out there and how their setup >> looks >> like. > > I'm also thinking about solving this issue by adding new APIs to the > HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware > drivers to call instead of netif_stop_queue / netif_wake_queue. This > way we can preserve backward compatibility. > > However I'm reluctant to change the code of all the hardware drivers > because I'm afraid of introducing bugs, etc. When I look at the code > of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no > bugs (related to stop_queue / wake_queue) after my change (and even > before my change, actually). There are even serious style problems: > the majority of its lines are indented by spaces. > > So I don't want to mess with all the hardware drivers. Hardware driver > developers (if they wish to properly support hdlc_x25) should do the > change themselves. This is not a problem for me, because I use my own > out-of-tree hardware driver. However if I add APIs with no user code > in the kernel, other developers may think these APIs are not > necessary. I don't think a change that affects the entire HDLC subsystem is justified, since the actual problem only affects the hdlc_x25 area. The approach with the additional hdlc<x>_x25 is clean and purposeful and I personally could live with it. I just don't see myself in the position to decide such a change at the moment. @Jakub: What is your opinion on this.
On Tue, 02 Mar 2021 08:04:20 +0100 Martin Schiller wrote: > On 2021-03-01 09:56, Xie He wrote: > > On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller <ms@dev.tdt.de> wrote: > >> I mean the change from only one hdlc<x> interface to both hdlc<x> and > >> hdlc<x>_x25. > >> > >> I can't estimate how many users are out there and how their setup > >> looks > >> like. > > > > I'm also thinking about solving this issue by adding new APIs to the > > HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware > > drivers to call instead of netif_stop_queue / netif_wake_queue. This > > way we can preserve backward compatibility. > > > > However I'm reluctant to change the code of all the hardware drivers > > because I'm afraid of introducing bugs, etc. When I look at the code > > of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no > > bugs (related to stop_queue / wake_queue) after my change (and even > > before my change, actually). There are even serious style problems: > > the majority of its lines are indented by spaces. > > > > So I don't want to mess with all the hardware drivers. Hardware driver > > developers (if they wish to properly support hdlc_x25) should do the > > change themselves. This is not a problem for me, because I use my own > > out-of-tree hardware driver. However if I add APIs with no user code > > in the kernel, other developers may think these APIs are not > > necessary. > > I don't think a change that affects the entire HDLC subsystem is > justified, since the actual problem only affects the hdlc_x25 area. > > The approach with the additional hdlc<x>_x25 is clean and purposeful and > I personally could live with it. > > I just don't see myself in the position to decide such a change at the > moment. > > @Jakub: What is your opinion on this. Hard question to answer, existing users seem happy and Xie's driver isn't upstream, so the justification for potentially breaking backward compatibility isn't exactly "strong". Can we cop out and add a knob somewhere to control spawning the extra netdev? Let people who just want a newer kernel carry on without distractions and those who want the extra layer can flip the switch?
On 2021-03-03 00:30, Jakub Kicinski wrote: > On Tue, 02 Mar 2021 08:04:20 +0100 Martin Schiller wrote: >> On 2021-03-01 09:56, Xie He wrote: >> > On Sun, Feb 28, 2021 at 10:56 PM Martin Schiller <ms@dev.tdt.de> wrote: >> >> I mean the change from only one hdlc<x> interface to both hdlc<x> and >> >> hdlc<x>_x25. >> >> >> >> I can't estimate how many users are out there and how their setup >> >> looks >> >> like. >> > >> > I'm also thinking about solving this issue by adding new APIs to the >> > HDLC subsystem (hdlc_stop_queue / hdlc_wake_queue) for hardware >> > drivers to call instead of netif_stop_queue / netif_wake_queue. This >> > way we can preserve backward compatibility. >> > >> > However I'm reluctant to change the code of all the hardware drivers >> > because I'm afraid of introducing bugs, etc. When I look at the code >> > of "wan/lmc/lmc_main.c", I feel I'm not able to make sure there are no >> > bugs (related to stop_queue / wake_queue) after my change (and even >> > before my change, actually). There are even serious style problems: >> > the majority of its lines are indented by spaces. >> > >> > So I don't want to mess with all the hardware drivers. Hardware driver >> > developers (if they wish to properly support hdlc_x25) should do the >> > change themselves. This is not a problem for me, because I use my own >> > out-of-tree hardware driver. However if I add APIs with no user code >> > in the kernel, other developers may think these APIs are not >> > necessary. >> >> I don't think a change that affects the entire HDLC subsystem is >> justified, since the actual problem only affects the hdlc_x25 area. >> >> The approach with the additional hdlc<x>_x25 is clean and purposeful >> and >> I personally could live with it. >> >> I just don't see myself in the position to decide such a change at the >> moment. >> >> @Jakub: What is your opinion on this. > > Hard question to answer, existing users seem happy and Xie's driver > isn't upstream, so the justification for potentially breaking backward > compatibility isn't exactly "strong". > > Can we cop out and add a knob somewhere to control spawning the extra > netdev? Let people who just want a newer kernel carry on without > distractions and those who want the extra layer can flip the switch? Yes, that would be a good compromise. I think a compile time selection option is enough here. We could introduce a new config option CONFIG_HDLC_X25_LEGACY (or something like that) and then implement the new or the old behavior in the driver accordingly. A switch that can be toggled at runtime (e.g. via sethdlc) would also be conceivable, but I don't think this is necessary.
On Wed, Mar 3, 2021 at 5:26 AM Martin Schiller <ms@dev.tdt.de> wrote: > > On 2021-03-03 00:30, Jakub Kicinski wrote: > > > > Hard question to answer, existing users seem happy and Xie's driver > > isn't upstream, so the justification for potentially breaking backward > > compatibility isn't exactly "strong". > > > > Can we cop out and add a knob somewhere to control spawning the extra > > netdev? Let people who just want a newer kernel carry on without > > distractions and those who want the extra layer can flip the switch? > > Yes, that would be a good compromise. > I think a compile time selection option is enough here. > We could introduce a new config option CONFIG_HDLC_X25_LEGACY (or > something like that) and then implement the new or the old behavior in > the driver accordingly. > > A switch that can be toggled at runtime (e.g. via sethdlc) would also be > conceivable, but I don't think this is necessary. Yes, I think adding a new config option would be a good way. Thank you both!!
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..b7744065900f 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,101 @@ 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 = x25_open, + .ndo_stop = x25_close, + .ndo_start_xmit = x25_xmit, +}; + +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; +} + +static int x25_hdlc_open(struct net_device *dev) +{ + struct hdlc_device *hdlc = dev_to_hdlc(dev); + struct net_device *x25_dev; + char x25_dev_name[sizeof(x25_dev->name)]; + int result; + + if (strlen(dev->name) + 4 >= sizeof(x25_dev_name)) + return -EINVAL; + + strcpy(x25_dev_name, dev->name); + strcat(x25_dev_name, "_x25"); + + x25_dev = alloc_netdev(sizeof(struct x25_device), x25_dev_name, + NET_NAME_PREDICTABLE, x25_setup_virtual_dev); + if (!x25_dev) + return -ENOMEM; + + dev_to_x25(x25_dev)->hdlc_dev = dev; + + /* netif_carrier_on will be called later by x25_hdlc_start */ + netif_carrier_off(x25_dev); + + result = register_netdevice(x25_dev); + if (result) { + free_netdev(x25_dev); + return result; + } + + spin_lock_bh(&state(hdlc)->x25_dev_lock); + state(hdlc)->x25_dev = x25_dev; + spin_unlock_bh(&state(hdlc)->x25_dev_lock); + + return 0; +} + +static void x25_hdlc_close(struct net_device *dev) +{ + struct hdlc_device *hdlc = dev_to_hdlc(dev); + struct net_device *x25_dev = state(hdlc)->x25_dev; + + if (x25_dev->flags & IFF_UP) + dev_close(x25_dev); + + spin_lock_bh(&state(hdlc)->x25_dev_lock); + state(hdlc)->x25_dev = NULL; + spin_unlock_bh(&state(hdlc)->x25_dev_lock); + + unregister_netdevice(x25_dev); + free_netdev(x25_dev); +} + +static void x25_hdlc_start(struct net_device *dev) +{ + struct hdlc_device *hdlc = dev_to_hdlc(dev); + struct net_device *x25_dev = state(hdlc)->x25_dev; + + /* hdlc.c guarantees no racing so we're sure x25_dev is valid */ + netif_carrier_on(x25_dev); +} + +static void x25_hdlc_stop(struct net_device *dev) +{ + struct hdlc_device *hdlc = dev_to_hdlc(dev); + struct net_device *x25_dev = state(hdlc)->x25_dev; + + /* hdlc.c guarantees no racing so we're sure x25_dev is valid */ + netif_carrier_off(x25_dev); } @@ -205,27 +305,38 @@ static void x25_close(struct net_device *dev) static int x25_rx(struct sk_buff *skb) { struct net_device *dev = skb->dev; + struct hdlc_device *hdlc = dev_to_hdlc(dev); + struct net_device *x25_dev; if ((skb = skb_share_check(skb, GFP_ATOMIC)) == NULL) { dev->stats.rx_dropped++; return NET_RX_DROP; } - if (lapb_data_received(dev, skb) == LAPB_OK) - return NET_RX_SUCCESS; - - dev->stats.rx_errors++; + spin_lock_bh(&state(hdlc)->x25_dev_lock); + x25_dev = state(hdlc)->x25_dev; + if (!x25_dev) + goto drop; + if (lapb_data_received(x25_dev, skb) != LAPB_OK) + goto drop; + spin_unlock_bh(&state(hdlc)->x25_dev_lock); + return NET_RX_SUCCESS; + +drop: + spin_unlock_bh(&state(hdlc)->x25_dev_lock); + dev->stats.rx_dropped++; dev_kfree_skb_any(skb); return NET_RX_DROP; } static struct hdlc_proto proto = { - .open = x25_open, - .close = x25_close, + .open = x25_hdlc_open, + .close = x25_hdlc_close, + .start = x25_hdlc_start, + .stop = x25_hdlc_stop, .ioctl = x25_ioctl, .netif_rx = x25_rx, - .xmit = x25_xmit, .module = THIS_MODULE, }; @@ -298,16 +409,9 @@ static int x25_ioctl(struct net_device *dev, struct ifreq *ifr) return result; memcpy(&state(hdlc)->settings, &new_settings, size); + state(hdlc)->x25_dev = NULL; + spin_lock_init(&state(hdlc)->x25_dev_lock); - /* There's no header_ops so hard_header_len should be 0. */ - dev->hard_header_len = 0; - /* When transmitting data: - * first we'll remove a pseudo header of 1 byte, - * then we'll prepend an LAPB header of at most 3 bytes. - */ - dev->needed_headroom = 3 - 1; - - dev->type = ARPHRD_X25; call_netdevice_notifiers(NETDEV_POST_TYPE_CHANGE, dev); netif_dormant_off(dev); return 0;
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 <ms@dev.tdt.de> Signed-off-by: Xie He <xie.he.0141@gmail.com> --- Change from RFC v3: Call netif_carrier_off in x25_hdlc_open before calling register_netdevice. 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 | 156 ++++++++++++++++++---- 2 files changed, 133 insertions(+), 26 deletions(-)