Re: [PATCH -mm] x86 boot : export boot_params via sysfs (forward to Greg)

Re: [PATCH -mm] x86 boot : export boot_params via sysfs (forward to Greg)

[Greg KH]

On Wed, Dec 12, 2007 at 04:11:19PM +0800, Huang, Ying wrote:
> On Tue, 2007-12-11 at 23:35 -0800, Greg KH wrote:
> > > +static struct attribute *boot_params_attrs[] = {
> > > +	&boot_params_version_attr.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static struct attribute_group boot_params_attr_group = {
> > > +	.attrs = boot_params_attrs,
> > > +};
> > > +
> > > +static ssize_t boot_params_data_read(struct kobject *kobj,
> > > +				     struct bin_attribute *bin_attr,
> > > +				     char *buf, loff_t off, size_t count)
> > > +{
> > > +	memcpy(buf, (void *)&boot_params + off, count);
> > > +	return count;
> > > +}
> > 
> > Why is the whole structure being exported to userspace directly?
> > Traditionally we only use the binary files for blobs that are not
> > interpreted by the kernel in any way.  Is that what this structure
> > really is?
> 
> The struct boot_params is not the case. But it is too complex (such as
> the struct edd_info in it). It is very complex to output every fields
> include the first level fields until the primary data type. And Peter
> Anvin think it is OK to output it as a big binary file.

Well, I respectively disagree.  sysfs is NOT for exporting various
binary kernel structures to userspace directly.  Again, the binary files
in sysfs are for chunks of memory that are PASS-THROUGH from hardware to
userspace, with no kernel intervention at all.

If you really need such a thing, use debugfs, as the only rule for
debugfs is that there is no rules :)

So I do not agree with this change at this time, and do not want to see
it applied to the tree.

If you really need this kind of information exported to userspace
through sysfs, then expand it all, in a one-text-value-per-file
format.  I would have no objection to that.

thanks,

greg k-h

Re: [PATCH -mm] x86 boot : export boot_params via sysfs (forward to Greg)

[Dave Jones]

On Wed, Dec 12, 2007 at 09:45:07AM -0800, Greg Kroah-Hartman wrote:

 > Well, I respectively disagree.  sysfs is NOT for exporting various
 > binary kernel structures to userspace directly.  Again, the binary files
 > in sysfs are for chunks of memory that are PASS-THROUGH from hardware to
 > userspace, with no kernel intervention at all.
 > If you really need such a thing, use debugfs, as the only rule for
 > debugfs is that there is no rules :)

Whilst on the subject, why wasn't /sys/slab done in debugfs ?
The one-value-per-file thing has gone taken to ridiculous extremes there.

Having 3641 sysfs files that most people never use permanently taking up
memory seems to be a massive waste of resources.

Nearly a third of all the sysfs files I have on my system belong to that
subtree, which is just.. wow.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH -mm] x86 boot : export boot_params via sysfs (forward to Greg)

[Greg KH]

On Wed, Dec 12, 2007 at 02:15:35PM -0500, Dave Jones wrote:
> On Wed, Dec 12, 2007 at 09:45:07AM -0800, Greg Kroah-Hartman wrote:
> 
>  > Well, I respectively disagree.  sysfs is NOT for exporting various
>  > binary kernel structures to userspace directly.  Again, the binary files
>  > in sysfs are for chunks of memory that are PASS-THROUGH from hardware to
>  > userspace, with no kernel intervention at all.
>  > If you really need such a thing, use debugfs, as the only rule for
>  > debugfs is that there is no rules :)
> 
> Whilst on the subject, why wasn't /sys/slab done in debugfs ?
> The one-value-per-file thing has gone taken to ridiculous extremes there.

Heh, I think most of those files only show up if you have
CONFIG_SLUB_DEBUG enabled, and it's easier to add them to the original
sysfs entries instead of creating new debugfs ones.

> Having 3641 sysfs files that most people never use permanently taking up
> memory seems to be a massive waste of resources.

sysfs files have their dentries and inodes pushed out of memory if they
are never accessed, so no huge ammounts of memory are used here.

Otherwise we would never be able to support 20,000 block devices on a
31bit s390 with 128Mb of ram :)

thanks,

greg k-h