On Sun, Mar 19, 2017 at 12:41:20AM -0400, Oleg Drokin wrote: > > On Mar 19, 2017, at 12:29 AM, Greg Kroah-Hartman wrote: > > > On Sat, Mar 18, 2017 at 11:17:55AM -0400, Oleg Drokin wrote: > >> > >> On Mar 18, 2017, at 6:34 AM, Greg Kroah-Hartman wrote: > >> > >>> On Sat, Mar 18, 2017 at 02:24:08AM -0400, Oleg Drokin wrote: > >>>> Ever since sysfs migration, class_process_proc_param stopped working > >>>> correctly as all the useful params were no longer present as lvars. > >>>> Replace all the nasty fake proc writes with hopefully less nasty > >>>> kobject attribute search and then update the attributes as needed. > >>>> > >>>> Signed-off-by: Oleg Drokin <green@xxxxxxxxxxxxxx> > >>>> Reported-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > >>>> --- > >>>> Al has quite rightfully complained in the past that class_process_proc_param > >>>> is a terrible piece of code and needs to go. > >>>> This patch is an attempt at improving it somewhat and in process drop > >>>> all the user/kernel address space games we needed to play to make it work > >>>> in the past (and which I suspect attracted Al's attention in the first place). > >>>> > >>>> Now I wonder if iterating kobject attributes like that would be ok with > >>>> you Greg, or do you think there is a better way? > >>>> class_find_write_attr could be turned into something generic since it's > >>>> certainly convenient to reuse same table of name-write_method pairs, > >>>> but I did some cursory research and nobody else seems to need anything > >>>> of the sort in-tree. > >>>> > >>>> I know ll_process_config is still awful and I will likely just > >>>> replace the current hack with kset_find_obj, but I just wanted to make > >>>> sure this new approach would be ok before spending too much time on it. > >>> > >>> I'm not quite sure what exactly you are even trying to do here. What is > >>> this interface? Who calls it, and how? What does it want to do? > >> > >> This is a configuration client code. > >> Management server has ability to pass down config information in the form of: > >> fsname.subsystem.attribute=value to clients and other servers > >> (subsystem determines if it's something of interest of clients or servers or > >> both). > >> This could be changed in the real time - i.e. you update it on the server and > >> that gets propagated to all the clients/servers, so no need to ssh into > >> every node to manually apply those changes and worry about client restarts > >> (the config is remembered at the management server and would be applied to any > >> new nodes connecting/across server restarts and such). > >> > >> So the way it then works then is once the string fsname.subsystem.attribute=value is delivered to the client, we find all instances of filesystem with fsname and then > >> all subsystems within it (one kobject per subsystem instance) and then update the > >> attributes to the value supplied. > >> > >> The same filesystem might be mounted more than once and then some layers might have > >> multiple instances inside a single filesystems. > >> > >> In the end it would turn something like lustre.osc.max_dirty_mb=128 into > >> writes to > >> /sys/fs/lustre/osc/lustre-OST0000-osc-ffff8800d75ca000/max_dirty_mb and > >> /sys/fs/lustre/osc/lustre-OST0001-osc-ffff8800d75ca000/max_dirty_mb > >> without actually iterating in sysfs namespace. > > > > Wait, who is doing the "write"? From within the kernel? Or some > > userspace app? I'm guessing from within the kernel, you are receiving > > the data from some other transport within the filesystem and then need > > to apply the settings? > > Yes, kernel code gets the notification "hey, there's a config change, come get it", > then it requests the diff in the config and does the "write' by updating the > attributes. > > >> The alternative we considered is we can probably just do an upcall and have > >> a userspace tool called with the parameter verbatim and try to figure it out, > >> but that seems a lot less ideal, and also we'll get a bunch of complications from > >> containers and such too, I imagine. > > > > Yeah, no, don't do an upcall, that's a mess. > > > >> The function pre-this patch is assuming that all these values are part of > >> a list of procfs values (no longer true after sysfs migration) so just iterates > >> that list and calls the write for matched names (but also needs to supply a userspace > >> buffer so looks much uglier too). > > > > For kobjects, you don't need userspace buffers, so this should be > > easier. > > Right, it's less of a mess due to that. > > >> Hopefully this makes at least some sense. > > > > Yes, a bit more, but ugh, what a mess :) > > Overall big systems are quite messy in many ways no matter what you do, > so it's always a case of pick your poison. Oh I know, I wasn't complaining, it's just a fact of life :) > >>> You can look up attributes for a kobject easily in the show/store > >>> functions (and some drivers just have a generic one and then you look at > >>> the string to see which attribute you are wanting to reference.) But > >>> you seem to be working backwards here, why do you have to look up a > >>> kobject? > >> > >> But that leads to the need to list attribute names essentially twice: > >> once for the attributes list, once in the show/set function to figure > >> out how to deal with that name. > > > > Yes, you don't need/want to do that. > > > > Let me go review the code now with that info, thanks, it makes more > > sense. > > Thanks! > In the end if the approach is mostly acceptable, I imagine if we just register the > kobject for the config changes, we can do away with the individual layers calling the > class_process_attr_param and instead have the class_process_config() call it directly > more or less. Yes, that seems like it would work as well. thanks, greg k-h _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
