| Message ID | 20231026001050.1720612-1-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > This patchset adds Rust abstractions for phylib. It doesn't fully > cover the C APIs yet but I think that it's already useful. I implement > two PHY drivers (Asix AX88772A PHYs and Realtek Generic FE-GE). Seems > they work well with real hardware. This patch series has had 8 versions in a month. It would be better to wait more between revisions for this kind of patch series, especially when there is discussion still going on in the previous ones and it is a new "type" of code. I understand you are doing it so that people can see the latest version (and thus focus on that), and that is helpful, but sending versions very quickly when the discussions are ongoing splits the discussions into several versions. That makes it more confusing for reviewers, and essentially discourages people from being able to follow everything. That, in turn, contributes to the problem of reviewers repeating feedback or missing context -- which you said you did not like. Please see https://docs.kernel.org/process/submitting-patches.html#don-t-get-discouraged-or-impatient Thanks! Cheers, Miguel
On Thu, Oct 26, 2023 at 12:39:46PM +0200, Miguel Ojeda wrote: > On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > > This patchset adds Rust abstractions for phylib. It doesn't fully > > cover the C APIs yet but I think that it's already useful. I implement > > two PHY drivers (Asix AX88772A PHYs and Realtek Generic FE-GE). Seems > > they work well with real hardware. > > This patch series has had 8 versions in a month. It would be better to > wait more between revisions for this kind of patch series, especially > when there is discussion still going on in the previous ones and it is > a new "type" of code. That is actually about right for netdev. As i said, netdev moves fast, review comments are expected within about 3 days. We also say don't post a new version within 24 hours. So that gives you an idea of the min and max. It is however good to let discussion reach some sort of conclusion, but that also requires prompt discussion. And if that discussion is not prompt, posting a new version is a way to kick reviewers into action. Andrew
On Fri, Oct 27, 2023 at 01:48:42AM +0200, Andrew Lunn wrote: > On Thu, Oct 26, 2023 at 12:39:46PM +0200, Miguel Ojeda wrote: > > On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori > > <fujita.tomonori@gmail.com> wrote: > > > > > > This patchset adds Rust abstractions for phylib. It doesn't fully > > > cover the C APIs yet but I think that it's already useful. I implement > > > two PHY drivers (Asix AX88772A PHYs and Realtek Generic FE-GE). Seems > > > they work well with real hardware. > > > > This patch series has had 8 versions in a month. It would be better to > > wait more between revisions for this kind of patch series, especially > > when there is discussion still going on in the previous ones and it is > > a new "type" of code. > > That is actually about right for netdev. As i said, netdev moves fast, > review comments are expected within about 3 days. We also say don't > post a new version within 24 hours. So that gives you an idea of the > min and max. > > It is however good to let discussion reach some sort of conclusion, > but that also requires prompt discussion. And if that discussion is > not prompt, posting a new version is a way to kick reviewers into > action. > I wonder whether that actually helps, if a reviewer takes average four days to review a version (wants to give accurate comments and doesn't work on this full time), and the developer send a new version every three days, there is no possible way for the developer to get the reviews. (Honestly, if people could reach out to a conclusion for anything in three days, the world would be a much more peaceful place ;-)) Regards, Boqun > Andrew >
> I wonder whether that actually helps, if a reviewer takes average four > days to review a version (wants to give accurate comments and doesn't > work on this full time), and the developer send a new version every > three days, there is no possible way for the developer to get the > reviews. > > (Honestly, if people could reach out to a conclusion for anything in > three days, the world would be a much more peaceful place ;-)) May i suggest you subscribe to the netdev list and watch it in action. It should also be noted, patches don't need reviews to be merged. If there is no feedback within three days, and it passes the CI tests, it likely will be merged. Real problems can be fixed up later, if need be. Andrew
On Fri, Oct 27, 2023 at 04:47:17AM +0200, Andrew Lunn wrote: > > I wonder whether that actually helps, if a reviewer takes average four > > days to review a version (wants to give accurate comments and doesn't > > work on this full time), and the developer send a new version every > > three days, there is no possible way for the developer to get the > > reviews. > > > > (Honestly, if people could reach out to a conclusion for anything in > > three days, the world would be a much more peaceful place ;-)) > > May i suggest you subscribe to the netdev list and watch it in action. > I'm sorry, I wasn't questioning about the process of netdev, I respect the hard work behind that. I was simply wondering whether sending out a new version so quickly is a good way to kick reviewers... it's sometimes frustating to see new version post but old comments were not resolved, it rather discourages reviewers.. > It should also be noted, patches don't need reviews to be merged. If > there is no feedback within three days, and it passes the CI tests, it Do the CI tests support Rust now? Does Tomo's patch pass CI? Looks like something we'd like to see (and help). > likely will be merged. Real problems can be fixed up later, if need > be. But this doesn't apply to pure API, right? So if some one post a pure Rust API with no user, but some tests, and the CI passes, the API won't get merged? Even though no review is fine and if API has problems, we can fix it later? Regards, Boqun > > Andrew >
On Thu, Oct 26, 2023 at 08:11:00PM -0700, Boqun Feng wrote: [...] > > likely will be merged. Real problems can be fixed up later, if need > > be. > > But this doesn't apply to pure API, right? So if some one post a pure > Rust API with no user, but some tests, and the CI passes, the API won't > get merged? Even though no review is fine and if API has problems, we > can fix it later? > I brought this up because (at least) at this stage one of the focus areas of Rust is: how to wrap C structs and functions into a sound and reasonable API. So it's ideal if we can see more API proposals. As you may already see in the reviews of this patchset, a lot of effort was spent on reviewing the API based on its designed semantics (rather than its usage in the last patch). This is the extra effort of using Rust. Is it worth? I don't know, the experiment will answer that in the end ;-) But at least having an API design with a clear semantics and some future guards is great. The review because of this may take longer time than C code, so if we really want to keep up with netdev speed, maybe we relax the must-have-in-tree-user rule, so that we can review API design and soundness in our pace. Regards, Boqun
On Fri, Oct 27, 2023 at 1:48 AM Andrew Lunn <andrew@lunn.ch> wrote: > > That is actually about right for netdev. As i said, netdev moves fast, > review comments are expected within about 3 days. We also say don't > post a new version within 24 hours. So that gives you an idea of the > min and max. And as I said when you told us that, that may be the right policy for C netdev, which has been around for a long time, has a well supported infrastructure, the code base is well-known, etc. For Rust, it is not, for multiple reasons. For starters, we are not talking here about just another patch to your subsystem. This is a whole new subsystem API, in a new language, that needs careful design. Moreover, Rust abstractions are supposed to be "sound" (a property that C APIs do not need). On top of that, two subsystems are reviewing it, and on our side we simply do not have the resources to commit to netdev review pace, as we told you privately. I am aware of at least 3 reviewers who have not had the time to look into the more recent versions yet, and it is unlikely they will have time before LPC. I would really recommend they are given the chance to give feedback. So, if you appreciate/want/need our feedback, you will need to be a bit more patient. By the way, your docs say patches are triaged in less than 48 hours as well as "Generally speaking". From that wording, I wouldn't expect every single patch to take 48 hours to be fully reviewed (not just triaged), and I would say the "very first Rust abstractions code" is not a common situation. > It is however good to let discussion reach some sort of conclusion, > but that also requires prompt discussion. I don't see why that would require "prompt discussion". > And if that discussion is > not prompt, posting a new version is a way to kick reviewers into > action. No, sorry, posting a new version in order to push reviewers is not the right thing to do. The patches were not being ignored. From your own (netdev) docs -- not even the general kernel ones: "Do not post a new version of the code if the discussion about the previous version is still ongoing, unless directly instructed by a reviewer." "Asking the maintainer for status updates on your patch is a good way to ensure your patch is ignored or pushed to the bottom of the priority list." "Make sure you address all the feedback in your new posting." Cheers, Miguel
On Fri, Oct 27, 2023 at 4:47 AM Andrew Lunn <andrew@lunn.ch> wrote: > > It should also be noted, patches don't need reviews to be merged. If > there is no feedback within three days, and it passes the CI tests, it > likely will be merged. Real problems can be fixed up later, if need > be. Passing CI tests does not tell you whether abstractions are well-designed or sound, which is the key property Rust abstractions need. And I hope by "don't need reviews to be merged" you mean "at least somebody, perhaps the applier, has taken a look". Cheers, Miguel
> Do the CI tests support Rust now? Does Tomo's patch pass CI? Looks like > something we'd like to see (and help). Its work in progress. https://patchwork.kernel.org/project/netdevbpf/patch/20231026001050.1720612-6-fujita.tomonori@gmail.com/ These are the current tests we have. You can see it fails two tests. I would say neither are blockers. netdev does try to stick to 80 character line length, so it would be nice to fix that. The checkpatch warning about the Kconfig help can also be ignored. There are currently a few builds performed for each patch, once with gcc and once with clang/llvm. These use the allmodconfig kernel configuration, since that generally builds the most code. However, Rust is not enabled in the configuration. So i submitted a new test, based on the clang build, which massages the kernel configuration to actually enable Rust, and to ensure the dependencies are fulfilled to allow the PHY driver to be enabled, and then enable it. We want the build with a patch to have equal or less errors and warning from the toolchain. Its not clear how long it will take before this new test becomes active. The build machine does not have a Rust toolchain yet, etc. To make up for that, i just build the series myself on my machine, and it builds cleanly for me. We are also open to add more tests. You will get more return on investment if you extend the C=1 checks, since that is used in a lot more places than networking. But we can add more tests to the networking CI system, if you can tell us what to test, how to test it, and how to evaluate the results. > > likely will be merged. Real problems can be fixed up later, if need > > be. > > But this doesn't apply to pure API, right? So if some one post a pure > Rust API with no user, but some tests, and the CI passes, the API won't > get merged? Even though no review is fine and if API has problems, we > can fix it later? There is always a human involved. If a reviewer does not pick up the missing user, the Maintainer should and reject the patch. Andrew
On Fri, Oct 27, 2023 at 12:22:07PM +0200, Miguel Ojeda wrote: > On Fri, Oct 27, 2023 at 4:47 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > It should also be noted, patches don't need reviews to be merged. If > > there is no feedback within three days, and it passes the CI tests, it > > likely will be merged. Real problems can be fixed up later, if need > > be. > > Passing CI tests does not tell you whether abstractions are > well-designed or sound, which is the key property Rust abstractions > need. We see CI as a tool to do the boring stuff. Does the code even compile without adding errors/warnings. Does it have the needed signed-off-by, does checkpatch spot anything nasty going on. Part of being able to handle the volume of patches in netdev is automating all this sort of stuff. > And I hope by "don't need reviews to be merged" you mean "at least > somebody, perhaps the applier, has taken a look". A human is always involved, looking at the CI results, and if nothing bad is reported, looking at the code if there are no Acked-by, or Reviewed-by from trusted people. The API being unsound is just another bug. I nobody spots the problem it can be fixed, just like any other bug. Andrew
On Fri, 27 Oct 2023 12:21:33 +0200 Miguel Ojeda wrote: > And as I said when you told us that, that may be the right policy for > C netdev, which has been around for a long time, has a well supported > infrastructure, the code base is well-known, etc. > > For Rust, it is not, for multiple reasons. For starters, we are not > talking here about just another patch to your subsystem. This is a > whole new subsystem API, in a new language, that needs careful design. > Moreover, Rust abstractions are supposed to be "sound" (a property > that C APIs do not need). Nobody is questioning that. > On top of that, two subsystems are reviewing it, and on our side we > simply do not have the resources to commit to netdev review pace, as > we told you privately. I am aware of at least 3 reviewers who have not > had the time to look into the more recent versions yet, and it is > unlikely they will have time before LPC. I would really recommend they > are given the chance to give feedback. > > So, if you appreciate/want/need our feedback, you will need to be a > bit more patient. To be sure our process is not misunderstood - it's not about impatience (
On Thu, Oct 26, 2023 at 09:26:05PM -0700, Boqun Feng wrote: > On Thu, Oct 26, 2023 at 08:11:00PM -0700, Boqun Feng wrote: > [...] > > > likely will be merged. Real problems can be fixed up later, if need > > > be. > > > > But this doesn't apply to pure API, right? So if some one post a pure > > Rust API with no user, but some tests, and the CI passes, the API won't > > get merged? Even though no review is fine and if API has problems, we > > can fix it later? > > > > I brought this up because (at least) at this stage one of the focus > areas of Rust is: how to wrap C structs and functions into a sound and > reasonable API. So it's ideal if we can see more API proposals. > > As you may already see in the reviews of this patchset, a lot of effort > was spent on reviewing the API based on its designed semantics (rather > than its usage in the last patch). This is the extra effort of using > Rust. Is it worth? I don't know, the experiment will answer that in the > end ;-) But at least having an API design with a clear semantics and > some future guards is great. > > The review because of this may take longer time than C code, so if we > really want to keep up with netdev speed, maybe we relax the > must-have-in-tree-user rule, so that we can review API design and > soundness in our pace. The rule about having a user is there for a few reasons: Without seeing the user, you cannot say if the API makes sense. How is the user using the API? Is the architecture of the user correct? Fixing the architecture of the user could change the API. As an example, i've seen Rust wrapper on top of sockets. The read/write method use a plain block of memory. However, for a network filesystem, what you actually need to send is probably represented by a folio of pages in the buffer cache. Its more efficient to hand the folio over to the network stack. If the data is coming from user space, its probably coming via a socket. So its probably in the form of a list of chunks of data, maybe still in userspace, maybe with a header added in kernel space. You want to pass that representation to the stack, which can already handle it in an optimal way. A plain block of memory could in fact be correct, there are use cases where its simple command being sent to a modem. But its impossible to say what is correct unless you can use the user. Another point is that internal API are unstable. Any developer can change any API anywhere in the kernel, if they can justify it. That is an important feature of the kernel, and we don't like making it harder to change APIs. If you cannot see both sides of an API, its hard to know if you can change it, or how you need to change it. And at the moment, there are very few Rust developers. An API in Rust is going to be harder for a developer to change. So we have a strong reason to keep Rust APIs minimal, with clear users. But this API unstableness plays both ways. You don't need a perfect API before it is merged. You just need it good enough. You can keep working on it once its merged. If you missed something which makes is unsound, not a problem, its just a bug, fix it an move on, like any other bug. Andrew
On Fri, Oct 27, 2023 at 4:26 PM Jakub Kicinski <kuba@kernel.org> wrote: > > To be sure our process is not misunderstood - it's not about impatience > (
On Fri, Oct 27, 2023 at 4:26 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Without seeing the user, you cannot say if the API makes sense. (snip) We are aware of all that, thank you. But I am not sure what you are trying to point out. If I understand correctly, Boqun was just suggesting an idea to split the pace, not disputing the value of the rule or asking why it is in place. > But this API unstableness plays both ways. You don't need a perfect > API before it is merged. You just need it good enough. You can keep > working on it once its merged. Indeed, but there is no rush to put things in either. And for us, "good enough" so far (i.e. for the very first abstractions), means "everyone is in on the same page, no known soundness issues, well-designed API that can serve as an example, enough time to review, etc.". In other words, we are trying to build consensus, not just put code into the kernel. > If you missed something which makes is > unsound, not a problem, its just a bug, fix it an move on, like any > other bug. Sure, as long as you consider them stable-worthy issues and are happy taking patches that may need to substantially change an API to fix the unsoundness in some cases, that is fine. Cheers, Miguel
> For instance, as a trivial example, Andrew raised the maximum length > of a line in one of the last messages. We would like to avoid this > kind of difference between parts of the kernel -- it is the only > chance we will get, and there is really no reason to be inconsistent > (ideally, even automated, where possible). It should be noted that netdev prefers 80, which the coding standard expresses as the preferred value. checkpatch however now allows up to 100. The netdev CI job adds an extra parameter to checkpatch to enforce the preferred 80. You will probably find different levels of acceptance of 80 to 100 in different subsystems. So i'm not sure you will be able to achieve consistence. It should also be noted that 80, or 100, is not a strict limit. Being able to grep the kernel for strings is important. So the coding standard allows you to go passed this limit in order that you don't need to break a string. checkpatch understands this. I don't know if your automated tools support such exceptions. Andrew
On Sat, Oct 28, 2023 at 12:55 AM Andrew Lunn <andrew@lunn.ch> wrote: > > You will probably find different levels of acceptance of 80 to 100 in > different subsystems. So i'm not sure you will be able to achieve > consistence. I would definitely agree if this were C. I happen to maintain `.clang-format`, and it was an interesting process to approximate a "common enough" style as the base one. But for Rust, it is easy, because all the code is new. All Rust files are formatted the same way, across the entire kernel. > It should also be noted that 80, or 100, is not a strict limit. Being > able to grep the kernel for strings is important. So the coding > standard allows you to go passed this limit in order that you don't > need to break a string. checkpatch understands this. I don't know if > your automated tools support such exceptions. Not breaking string literals is the default behavior of `rustfmt` (and we use its default behavior). It is also definitely possible to turn off `rustfmt` locally, i.e. for particular "items" (e.g. a function, a block, a statement), rather than lines, which is very convenient. However, as far as I recall, we have never needed to disable it. I am sure it will eventually be needed somewhere, but what I am trying to say is that it works well enough that one can just use it. Cheers, Miguel
On 10/28/23 13:07, Miguel Ojeda wrote: > On Sat, Oct 28, 2023 at 12:55 AM Andrew Lunn <andrew@lunn.ch> wrote: >> It should also be noted that 80, or 100, is not a strict limit. Being >> able to grep the kernel for strings is important. So the coding >> standard allows you to go passed this limit in order that you don't >> need to break a string. checkpatch understands this. I don't know if >> your automated tools support such exceptions. > > Not breaking string literals is the default behavior of `rustfmt` (and > we use its default behavior). > > It is also definitely possible to turn off `rustfmt` locally, i.e. for > particular "items" (e.g. a function, a block, a statement), rather > than lines, which is very convenient. > > However, as far as I recall, we have never needed to disable it. I am > sure it will eventually be needed somewhere, but what I am trying to > say is that it works well enough that one can just use it. We have it disabled on the `pub mod code` in error.rs line 20: https://elixir.bootlin.com/linux/latest/source/rust/kernel/error.rs#L20
On Sat, Oct 28, 2023 at 01:07:43PM +0200, Miguel Ojeda wrote: > On Sat, Oct 28, 2023 at 12:55 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > You will probably find different levels of acceptance of 80 to 100 in > > different subsystems. So i'm not sure you will be able to achieve > > consistence. > > I would definitely agree if this were C. I happen to maintain > `.clang-format`, and it was an interesting process to approximate a > "common enough" style as the base one. > > But for Rust, it is easy, because all the code is new. All Rust files > are formatted the same way, across the entire kernel. I would say this is not a technical issue, but a social one. In order to keep the netdev people happy, you are going to limit it to 80. But i would not be too surprised if another subsystem says the code would be more readable with 100, like the C code in our subsystem. We want Rust to be 100 as well. Linux can be very fragmented like this across subsystems. Its just the way it is, and you might just have to fit in. I don't know, we will see. Andrew
On Sat, Oct 28, 2023 at 1:41 PM Benno Lossin <benno.lossin@proton.me> wrote: > > We have it disabled on the `pub mod code` in error.rs line 20: > https://elixir.bootlin.com/linux/latest/source/rust/kernel/error.rs#L20 Thanks Benno -- I checked the `rust` branch, and forgot about that one. It is an interesting example though, because it could have gone either way: we were not using it for the same code in the `rust` branch -- it was an aesthetic improvement for consistency. In fact, if we do the macro differently, it would not be needed. Cheers, Miguel
On Sat, Oct 28, 2023 at 5:01 PM Andrew Lunn <andrew@lunn.ch> wrote: > > I would say this is not a technical issue, but a social one. In order > to keep the netdev people happy, you are going to limit it to 80. But > i would not be too surprised if another subsystem says the code would > be more readable with 100, like the C code in our subsystem. We want > Rust to be 100 as well. To be clear, we don't really care about 80, 100, or 120, or tabs vs. spaces. What we want is consistency, and it was decided to go with the defaults of `rustfmt`. We may want to tweak a knob here or there in the future, but it would still apply to every single file. > Linux can be very fragmented like this across subsystems. Its just the > way it is, and you might just have to fit in. I don't know, we will > see. Yes, but that is an artifact of history and the tools used at the time. We can improve things now, thus why it was decided to do it consistently for all Rust code. Cheers, Miguel