Message ID | 20231205223118.3575485-1-souravpanda@google.com (mailing list archive) |
---|---|
Headers | show |
Series | mm: report per-page metadata information | expand |
On Tue, Dec 05, 2023 at 02:31:17PM -0800, Sourav Panda wrote: > Changelog: > v6: > - Interface changes > - Added per-node nr_page_metadata and > nr_page_metadata_boot fields that are exported > in /sys/devices/system/node/nodeN/vmstat Again, please do not add any new fields to existing sysfs files, that's not going to work well. You can add a new sysfs file, that's file, but do not continue the abuse of the sysfs api in this file. thanks, greg k-h
Hi Greg, Sourav removed the new field from sys/device../nodeN/meminfo as you requested; however, in nodeN/vmstat fields still get appended, as there is code that displays every item in zone_stat_item, node_stat_item without option to opt-out. I mentioned it to you at LPC. In my IOMMU [1] series, there are also fields that are added to node_stat_item that as result are appended to nodeN/vmstat. Pasha [1] https://lore.kernel.org/all/20231130201504.2322355-1-pasha.tatashin@soleen.com On Tue, Dec 5, 2023 at 9:36 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Dec 05, 2023 at 02:31:17PM -0800, Sourav Panda wrote: > > Changelog: > > v6: > > - Interface changes > > - Added per-node nr_page_metadata and > > nr_page_metadata_boot fields that are exported > > in /sys/devices/system/node/nodeN/vmstat > > Again, please do not add any new fields to existing sysfs files, that's > not going to work well. You can add a new sysfs file, that's file, but > do not continue the abuse of the sysfs api in this file. > > thanks, > > greg k-h
On Tue, Dec 05, 2023 at 09:57:36PM -0500, Pasha Tatashin wrote: > Hi Greg, > > Sourav removed the new field from sys/device../nodeN/meminfo as you > requested; however, in nodeN/vmstat fields still get appended, as > there is code that displays every item in zone_stat_item, > node_stat_item without option to opt-out. I mentioned it to you at > LPC. Sorry, I thought that was a proc file, not a sysfs file. Don't grow that file please, it should not be that way and adding to it will just cause others to also want to add to it and we end up with the whole proc file mess... > In my IOMMU [1] series, there are also fields that are added to > node_stat_item that as result are appended to nodeN/vmstat. I missed that, that too shouldn't be done please. Again, sysfs is "one value per file" for a reason, don't abuse the fact that we missed this abuse of the rules for that file by adding more things to it. thanks, greg k-h
On Wed, 6 Dec 2023 12:12:10 +0900 Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Dec 05, 2023 at 09:57:36PM -0500, Pasha Tatashin wrote: > > Hi Greg, > > > > Sourav removed the new field from sys/device../nodeN/meminfo as you > > requested; however, in nodeN/vmstat fields still get appended, as > > there is code that displays every item in zone_stat_item, > > node_stat_item without option to opt-out. I mentioned it to you at > > LPC. > > Sorry, I thought that was a proc file, not a sysfs file. Don't grow > that file please, it should not be that way and adding to it will just > cause others to also want to add to it and we end up with the whole proc > file mess... > > > In my IOMMU [1] series, there are also fields that are added to > > node_stat_item that as result are appended to nodeN/vmstat. > > I missed that, that too shouldn't be done please. > > Again, sysfs is "one value per file" for a reason, don't abuse the fact > that we missed this abuse of the rules for that file by adding more > things to it. I'm afraid that horse has bolted. hp2:/usr/src/25> wc /sys/devices/system/node/node0/vmstat 61 122 1309 /sys/devices/system/node/node0/vmstat We're never going to unpick this into 61 separate files so adding new files at this stage is pointless.
On Wed, Dec 06, 2023 at 07:59:13AM -0800, Andrew Morton wrote: > On Wed, 6 Dec 2023 12:12:10 +0900 Greg KH <gregkh@linuxfoundation.org> wrote: > > > On Tue, Dec 05, 2023 at 09:57:36PM -0500, Pasha Tatashin wrote: > > > Hi Greg, > > > > > > Sourav removed the new field from sys/device../nodeN/meminfo as you > > > requested; however, in nodeN/vmstat fields still get appended, as > > > there is code that displays every item in zone_stat_item, > > > node_stat_item without option to opt-out. I mentioned it to you at > > > LPC. > > > > Sorry, I thought that was a proc file, not a sysfs file. Don't grow > > that file please, it should not be that way and adding to it will just > > cause others to also want to add to it and we end up with the whole proc > > file mess... > > > > > In my IOMMU [1] series, there are also fields that are added to > > > node_stat_item that as result are appended to nodeN/vmstat. > > > > I missed that, that too shouldn't be done please. > > > > Again, sysfs is "one value per file" for a reason, don't abuse the fact > > that we missed this abuse of the rules for that file by adding more > > things to it. > > I'm afraid that horse has bolted. > > hp2:/usr/src/25> wc /sys/devices/system/node/node0/vmstat > 61 122 1309 /sys/devices/system/node/node0/vmstat > > We're never going to unpick this into 61 separate files so adding new > files at this stage is pointless. But if it keeps growing, it will eventually overflow and start crashing the kernel and you will then have to do the horrid thing of turning it into a binary sysfs file. So I can please ask that no new entries be added to the file please, let's not keep making things worse. For new items, just add new files, don't add to the existing mess. thanks, greg k-h