| Message ID | 20241228-sysfs-const-bin_attr-simple-v2-0-7c6f3f1767a3@weissschuh.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read() | expand |
On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > Most users use this function through the BIN_ATTR_SIMPLE* macros, > they can handle the switch transparently. > > This series is meant to be merged through the driver core tree. hmm. why? I'd rather take patches 2 and 3 into bpf-next to avoid potential conflicts. Patch 1 looks orthogonal and independent.
On 2024-12-30 16:50:41-0800, Alexei Starovoitov wrote: > On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > Most users use this function through the BIN_ATTR_SIMPLE* macros, > > they can handle the switch transparently. > > > > This series is meant to be merged through the driver core tree. > > hmm. why? Patch 1 changes the signature of sysfs_bin_attr_simple_read(). Before patch 1 sysfs_bin_attr_simple_read() needs to be assigned to the callback member .read, after patch 1 it's .read_new. (Both callbacks work exactly the same, except for their signature, .read_new is only a transition mechanism and will go away again) > I'd rather take patches 2 and 3 into bpf-next to avoid > potential conflicts. > Patch 1 looks orthogonal and independent. If you pick up 2 and 3 through bpf-next you would need to adapt these assignments. As soon as both patch 1 and the modified 2 and 3 hit Linus' tree, the build would break due to mismatches function pointers. (Casting function pointers to avoid the mismatch will blow up with KCFI) Of course Linus can fix this up easily, but it somebody would need to keep track of it and I wanted to avoid manual intervention. Or we spread out both parts over two development cycles; maybe Greg can even pick up patch 1 late in the 6.13 cycle. Personally I am fine with any approach.
On Tue, Dec 31, 2024 at 2:30 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > On 2024-12-30 16:50:41-0800, Alexei Starovoitov wrote: > > On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > Most users use this function through the BIN_ATTR_SIMPLE* macros, > > > they can handle the switch transparently. > > > > > > This series is meant to be merged through the driver core tree. > > > > hmm. why? > > Patch 1 changes the signature of sysfs_bin_attr_simple_read(). > Before patch 1 sysfs_bin_attr_simple_read() needs to be assigned to the > callback member .read, after patch 1 it's .read_new. > (Both callbacks work exactly the same, except for their signature, > .read_new is only a transition mechanism and will go away again) > > > I'd rather take patches 2 and 3 into bpf-next to avoid > > potential conflicts. > > Patch 1 looks orthogonal and independent. > > If you pick up 2 and 3 through bpf-next you would need to adapt these > assignments. As soon as both patch 1 and the modified 2 and 3 hit > Linus' tree, the build would break due to mismatches function pointers. > (Casting function pointers to avoid the mismatch will blow up with KCFI) I see. All these steps to constify is frankly a mess. You're wasting cpu and memory for this read vs read_new when const is not much more than syntactic sugar in C. You should have done one tree wide patch without doing this _new() hack. Anyway, rant over. Carry patches 2,3. Hopefully they won't conflict. But I don't want to see any constification patches in bpf land that come with such pointless runtime penalty.
On Wed, Jan 08, 2025 at 09:45:45AM -0800, Alexei Starovoitov wrote: > On Tue, Dec 31, 2024 at 2:30 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > On 2024-12-30 16:50:41-0800, Alexei Starovoitov wrote: > > > On Sat, Dec 28, 2024 at 12:43 AM Thomas Weißschuh <linux@weissschuh.net> wrote: > > > > > > > > Most users use this function through the BIN_ATTR_SIMPLE* macros, > > > > they can handle the switch transparently. > > > > > > > > This series is meant to be merged through the driver core tree. > > > > > > hmm. why? > > > > Patch 1 changes the signature of sysfs_bin_attr_simple_read(). > > Before patch 1 sysfs_bin_attr_simple_read() needs to be assigned to the > > callback member .read, after patch 1 it's .read_new. > > (Both callbacks work exactly the same, except for their signature, > > .read_new is only a transition mechanism and will go away again) > > > > > I'd rather take patches 2 and 3 into bpf-next to avoid > > > potential conflicts. > > > Patch 1 looks orthogonal and independent. > > > > If you pick up 2 and 3 through bpf-next you would need to adapt these > > assignments. As soon as both patch 1 and the modified 2 and 3 hit > > Linus' tree, the build would break due to mismatches function pointers. > > (Casting function pointers to avoid the mismatch will blow up with KCFI) > > I see. All these steps to constify is frankly a mess. > You're wasting cpu and memory for this read vs read_new > when const is not much more than syntactic sugar in C. > You should have done one tree wide patch without doing this _new() hack. > > Anyway, rant over. Carry patches 2,3. Hopefully they won't conflict. > But I don't want to see any constification patches in bpf land > that come with such pointless runtime penalty. The "pointless" penalty will go away once we convert all instances, and really, it's just one pointer check, sysfs files should NOT be a hot path for anything real, and one more pointer check should be cached and not measurable compared to the real logic behind the binary data coming from the hardware/kernel, right? sysfs is NOT tuned for speed at all, so adding more checks like this should be fine. thanks, greg k-h
On Thu, Jan 09, 2025 at 08:56:37AM +0100, Greg Kroah-Hartman wrote: > The "pointless" penalty will go away once we convert all instances, and > really, it's just one pointer check, sysfs files should NOT be a hot > path for anything real, and one more pointer check should be cached and > not measurable compared to the real logic behind the binary data coming > from the hardware/kernel, right? > > sysfs is NOT tuned for speed at all, so adding more checks like this > should be fine. Hey, when I duplicated the method to convert sysfs over to a proper seq_file based approach that avoids buffer overflows you basically came up with the same line that Alexei had here. And that is a lot more useful than constification. Not that I mind the latter, but it would be better if it could be done without leaving both variants in for long.
On Thu, Jan 09, 2025 at 12:06:09AM -0800, Christoph Hellwig wrote: > On Thu, Jan 09, 2025 at 08:56:37AM +0100, Greg Kroah-Hartman wrote: > > The "pointless" penalty will go away once we convert all instances, and > > really, it's just one pointer check, sysfs files should NOT be a hot > > path for anything real, and one more pointer check should be cached and > > not measurable compared to the real logic behind the binary data coming > > from the hardware/kernel, right? > > > > sysfs is NOT tuned for speed at all, so adding more checks like this > > should be fine. > > Hey, when I duplicated the method to convert sysfs over to a proper > seq_file based approach that avoids buffer overflows you basically > came up with the same line that Alexei had here. I did? Sorry about that, I don't remember that. > And that is a lot > more useful than constification. Not that I mind the latter, but it > would be better if it could be done without leaving both variants > in for long. I agree, we should get the read_new stuff out in the next kernel cycle I hope. As for seq_file for sysfs, is that for binary attributes only, or for all? I can't recall that at all. thanks, greg k-h
On Thu, Jan 09, 2025 at 09:12:03AM +0100, Greg Kroah-Hartman wrote: > > Hey, when I duplicated the method to convert sysfs over to a proper > > seq_file based approach that avoids buffer overflows you basically > > came up with the same line that Alexei had here. > > I did? Sorry about that, I don't remember that. It's been a while.. > As for seq_file for sysfs, is that for binary attributes only, or for > all? I can't recall that at all. Non-binary ones.
On Thu, Jan 09, 2025 at 12:23:01AM -0800, Christoph Hellwig wrote: > On Thu, Jan 09, 2025 at 09:12:03AM +0100, Greg Kroah-Hartman wrote: > > > Hey, when I duplicated the method to convert sysfs over to a proper > > > seq_file based approach that avoids buffer overflows you basically > > > came up with the same line that Alexei had here. > > > > I did? Sorry about that, I don't remember that. > > It's been a while.. > > > As for seq_file for sysfs, is that for binary attributes only, or for > > all? I can't recall that at all. > > Non-binary ones. Ah, yeah, well the churn for "one single value" sysfs files would be rough and seq_file doesn't really do much, if anything, for them as they should be all simple strings that never overflow or are complex. Yes, there are exceptions, so maybe for just them? I don't want to make it easier to abuse sysfs files, but if you feel it would really help out, I'm willing to reconsider it. thanks, greg k-h >
Most users use this function through the BIN_ATTR_SIMPLE* macros, they can handle the switch transparently. This series is meant to be merged through the driver core tree. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- Changes in v2: - Rebase on torvalds/master - Drop wmi-bmof patch - Pick up Acks from Andrii - Link to v1: https://lore.kernel.org/r/20241205-sysfs-const-bin_attr-simple-v1-0-4a4e4ced71e3@weissschuh.net --- Thomas Weißschuh (3): sysfs: constify bin_attribute argument of sysfs_bin_attr_simple_read() btf: Switch vmlinux BTF attribute to sysfs_bin_attr_simple_read() btf: Switch module BTF attribute to sysfs_bin_attr_simple_read() arch/powerpc/platforms/powernv/opal.c | 2 +- fs/sysfs/file.c | 2 +- include/linux/sysfs.h | 4 ++-- kernel/bpf/btf.c | 15 ++------------- kernel/bpf/sysfs_btf.c | 12 ++---------- kernel/module/sysfs.c | 2 +- 6 files changed, 9 insertions(+), 28 deletions(-) --- base-commit: d6ef8b40d075c425f548002d2f35ae3f06e9cf96 change-id: 20241122-sysfs-const-bin_attr-simple-7c0ddb2fcf12 Best regards,