| Message ID | 20221019132503.6783-1-dnlplm@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: usb: qmi_wwan implement tx packets aggregation | expand |
Daniele Palmas <dnlplm@gmail.com> writes: > I verified this problem by using a MDM9207 Cat. 4 based modem (50Mbps/150Mbps > max throughput). What is actually happening is pictured at > https://drive.google.com/file/d/1xuAuDBfBEIM3Cdg2zHYQJ5tdk-JkfQn7/view?usp=sharing > > When rx and tx flows are tested singularly there's no issue in tx and minor > issues in rx (a few spikes). When there are concurrent tx and rx flows, tx > throughput has an huge drop. rx a minor one, but still present. > > The same scenario with tx aggregation enabled is pictured at > https://drive.google.com/file/d/1Kw8TVFLVgr31o841fRu4fuMX9DNZqJB5/view?usp=sharing > showing a regular graph. That's pretty extreme. Are these tcp tests? Did you experiment with qdisc options? What about latency here? > This issue does not happen with high-cat modems (e.g. SDX20), or at least it > does not happen at the throughputs I'm able to test currently: maybe the same > could happen when moving close to the maximum rates supported by those modems. > Anyway, having the tx aggregation enabled should not hurt. > > It is interesting to note that, for what I can understand, rmnet too does not > support tx aggregation. Looks like that is missing, yes. Did you consider implementing it there instead? > I'm aware that rmnet should be the preferred way for qmap, but I think there's > still value in adding this feature to qmi_wwan qmap implementation since there > are in the field many users of that. > > Moreover, having this in mainline could simplify backporting for those who are > using qmi_wwan qmap feature but are stuck with old kernel versions. > > I'm also aware of the fact that sysfs files for configuration are not the > preferred way, but it would feel odd changing the way for configuring the driver > just for this feature, having it different from the previous knobs. It's not just that it's not the preferred way.. I believe I promised that we wouldn't add anything more to this interface. And then I broke that promise, promising that it would never happen again. So much for my integrity. This all looks very nice to me, and the results are great, and it's just another knob... But I don't think we can continue adding this stuff. The QMAP handling should be done in the rmnet driver. Unless there is some reason it can't be there? Wouldn't the same code work there? Luckily I can chicken out here, and leave final the decision to Jakub and David. Bjørn
On Wed, Oct 19, 2022 at 05:04:31PM +0200, Bjørn Mork wrote: > Daniele Palmas <dnlplm@gmail.com> writes: > > I'm aware that rmnet should be the preferred way for qmap, but I think there's > > still value in adding this feature to qmi_wwan qmap implementation since there > > are in the field many users of that. > > > > Moreover, having this in mainline could simplify backporting for those who are > > using qmi_wwan qmap feature but are stuck with old kernel versions. > > > > I'm also aware of the fact that sysfs files for configuration are not the > > preferred way, but it would feel odd changing the way for configuring the driver > > just for this feature, having it different from the previous knobs. > > It's not just that it's not the preferred way.. I believe I promised > that we wouldn't add anything more to this interface. And then I broke > that promise, promising that it would never happen again. So much for > my integrity. > > This all looks very nice to me, and the results are great, and it's just > another knob... > Please no more sysfs files for stuff like this. This turns into vendor-specific random files that no one knows how to change over time with no way to know what userspace tools are accessing them, or if even anyone is using them at all. Shouldn't there be standard ethtool apis for this? > But I don't think we can continue adding this stuff. The QMAP handling > should be done in the rmnet driver. Unless there is some reason it can't > be there? Wouldn't the same code work there? rmnet would be better, but again, no new sysfs files please, thanks, greg k-h
Il giorno mer 19 ott 2022 alle ore 17:04 Bjørn Mork <bjorn@mork.no> ha scritto: > > Daniele Palmas <dnlplm@gmail.com> writes: > > > I verified this problem by using a MDM9207 Cat. 4 based modem (50Mbps/150Mbps > > max throughput). What is actually happening is pictured at > > https://drive.google.com/file/d/1xuAuDBfBEIM3Cdg2zHYQJ5tdk-JkfQn7/view?usp=sharing > > > > When rx and tx flows are tested singularly there's no issue in tx and minor > > issues in rx (a few spikes). When there are concurrent tx and rx flows, tx > > throughput has an huge drop. rx a minor one, but still present. > > > > The same scenario with tx aggregation enabled is pictured at > > https://drive.google.com/file/d/1Kw8TVFLVgr31o841fRu4fuMX9DNZqJB5/view?usp=sharing > > showing a regular graph. > > That's pretty extreme. Are these tcp tests? Did you experiment with > qdisc options? What about latency here? > Yes, tcp with iperf. I did not try qdisc and haven't measured (yet) latency. > > This issue does not happen with high-cat modems (e.g. SDX20), or at least it > > does not happen at the throughputs I'm able to test currently: maybe the same > > could happen when moving close to the maximum rates supported by those modems. > > Anyway, having the tx aggregation enabled should not hurt. > > > > It is interesting to note that, for what I can understand, rmnet too does not > > support tx aggregation. > > Looks like that is missing, yes. Did you consider implementing it there > instead? > Yes, I thought about it, but it's something that has a broader impact, since it's not used just with usb, not really comfortable with that code, but I agree that's the way to go... > > I'm aware that rmnet should be the preferred way for qmap, but I think there's > > still value in adding this feature to qmi_wwan qmap implementation since there > > are in the field many users of that. > > > > Moreover, having this in mainline could simplify backporting for those who are > > using qmi_wwan qmap feature but are stuck with old kernel versions. > > > > I'm also aware of the fact that sysfs files for configuration are not the > > preferred way, but it would feel odd changing the way for configuring the driver > > just for this feature, having it different from the previous knobs. > > It's not just that it's not the preferred way.. I believe I promised > that we wouldn't add anything more to this interface. And then I broke > that promise, promising that it would never happen again. So much for > my integrity. > > This all looks very nice to me, and the results are great, and it's just > another knob... > > But I don't think we can continue adding this stuff. The QMAP handling > should be done in the rmnet driver. Unless there is some reason it can't > be there? Wouldn't the same code work there? > Ok, I admit that your reasoning makes sense. There's no real reason for not having tx aggregation in rmnet, besides the fact that no one has added it yet. There's some downstream code for example at https://source.codeaurora.org/quic/la/kernel/msm-4.19/tree/drivers/net/ethernet/qualcomm/rmnet/rmnet_handlers.c?h=LA.UM.8.12.3.1#n405 I can try looking at that to see if I'm able to implement the same feature in mainline rmnet. Thanks for your comments! Regards, Daniele > Luckily I can chicken out here, and leave final the decision to Jakub > and David. > > > > Bjørn
On Wed, 19 Oct 2022 17:48:35 +0200 Greg KH wrote: > > It's not just that it's not the preferred way.. I believe I promised > > that we wouldn't add anything more to this interface. And then I broke > > that promise, promising that it would never happen again. So much for > > my integrity. > > > > This all looks very nice to me, and the results are great, and it's just > > another knob... > > > > Please no more sysfs files for stuff like this. This turns into > vendor-specific random files that no one knows how to change over time > with no way to know what userspace tools are accessing them, or if even > anyone is using them at all. > > Shouldn't there be standard ethtool apis for this? Not yet, but there should. We can add the new params to struct kernel_ethtool_coalesce, and plumb them thru ethtool netlink. No major surgery required. Feel free to ask for more guidance if the netlink-y stuff is confusing.