Re: [Patch 5/8] taskstats interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Jay Lan wrote:

Hi Shailabh,

Thanks for your effort in taskstats interface! Really appreciated!
I think this interface can offer a good foundation for other packages
to build on.

Here are a few more comments:

1) You mentioned the "version number within the (taskstats)
   structure" in taskstats.txt and a few other places, but i do not see
   that field defined in struct taskstats in taskstats.h?
Missed out on that. Need to add it back in.

2) In taskstats.txt "Extending taskstats" section, you mentioned two
   ways to extend the interface. The second method looks like a method
   to encoureage other package developers to create their own interface
  (ie, not taskstats) based on generic netlink to avoid reading large
number
   of fields not interested to other particular applications? I will be
fine
   with this as long as it is understood and agreed.
Yes, the second method is for other packages, which have very little in common with the struct taskstats to extend the stats returned (using netlink attribs to extend rather than extending the structure).

   Alternatively, you may have considered the pros and cons of #ifdef
   fields specific to only one accounting package in the struct taskstats.
If you do, care to share your thoughts?
I'd rather avoid doing an #ifdef'ed definition of the fields based on configuration of one or the other accounting package...it'll add complexity for userspace parsing of the structure.

Its quite acceptable to have the fields have zero as content if the corresponding package isn't configured.


Specific payload information
   can be carried in the version field. I am sure the version number of
struct
   taskstats does not need 64 bits. With the version number and payload
info, application can surely interpret the taskstats data correctly.
By "payload info" you mean some sort of bitmask (or encoding) which specifies which fields are present or absent ? I suppose that could be done but it adds unnecessary complexity ? e.g once delay accounting is there, all six to eight fields corresponding to it will be present...I don't see much value in further being able to configure
cpu delays, mem delays etc. separately. Is that different for CSA ?


3) In taskstats.txt "Usage" section, you mentioned "... in the Advanced
   Usage section below...", but that section does not exist.
Thanks for pointing it out. Should replace it with "per-tgid stats section".

4) In do_exit() routine, you do:
+ taskstats_exit_alloc(&tidstats, &tgidstats);

   The tidstats and tgidstats are checked in taskstats_exit_send() in
   taskstats.c for allocation failure, but a lot has been processed before
   the check. The allocation failure happens when system is stressed in
   memory. I  think we want to do the check earlier?
Since accounting is non-critical, I didn't see the need for doing the check earlier if we're not going to do anything about it. The first use of the allocated structure is in the taskstats_exit_send() where filling of the stats is not done if allocation failed. What would you suggest we do, on allocation failure, if the check is
performed immediately after the alloc ?

--Shailabh

Regards,
- jay


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux