[libresource-devel] [PATCH] library of interfaces to fetch system resource information

Rahul Yadav rahul.x.yadav at oracle.com
Thu Mar 1 20:48:09 UTC 2018


Hi Jon,

Thanks for initial review, I have provided comments below. I will make 
changes in code
and README etc as mentioned at few places in my reply.

Thanks
Rahul

On 3/1/2018 11:55 AM, Jonathan Helman wrote:
> Hi Rahul,
>
> I have a few questions and comments below on the resource.h API and 
> README. Did not check out the implementation yet.
>
> Jon
>
> On 02/28/2018 10:58 PM, Rahul Yadav wrote:
>> 1: System resource Information related to memory, CPU, stat, networking,
>> security etc.
>> 2: Currently most of such information is read from /proc and /sys.
>> 3: Add basic infrastructure and code for some of memory and network 
>> related
>> information.
>> 4: Update README file with the use cases and design details.
>> 5: Added example codes in README.
>>
>> Signed-off-by: Rahul Yadav <rahul.x.yadav at oracle.com>
>> ---
>>   Makefile        |  18 ++++
>>   README.md       | 160 ++++++++++++++++++++++++++--
>>   resmem.c        | 218 ++++++++++++++++++++++++++++++++++++++
>>   resmem.h        |  16 +++
>>   resnet.c        | 316 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   resnet.h        |  14 +++
>>   resource.c      | 211 +++++++++++++++++++++++++++++++++++++
>>   resource.h      | 147 ++++++++++++++++++++++++++
>>   resource_impl.h |  47 +++++++++
>>   9 files changed, 1137 insertions(+), 10 deletions(-)
>>   create mode 100644 Makefile
>>   create mode 100644 resmem.c
>>   create mode 100644 resmem.h
>>   create mode 100644 resnet.c
>>   create mode 100644 resnet.h
>>   create mode 100644 resource.c
>>   create mode 100644 resource.h
>>   create mode 100644 resource_impl.h
>>
>> diff --git a/Makefile b/Makefile
>> new file mode 100644
>> index 0000000..2c71328
>> --- /dev/null
>> +++ b/Makefile
>> @@ -0,0 +1,18 @@
>> +CC = gcc
>> +CFLAGS = -g -Wall -Werror -fPIC
>> +DEPS = resource.h res_impl.h resmem.h resnet.h
>> +OBJ = resource.o resmem.o resnet.o
>> +LIB = libresource.so
>> +TEST = test
>> +RM = rm -rf
>> +CP = cp
>> +
>> +%.o: %.c $(DEPS)
>> +    $(CC) -c -o $@ $< $(CFLAGS)
>> +
>> +all: $(OBJ)
>> +    $(CC) -shared -o $(LIB) $^ $(CFLAGS)
>> +
>> +.PHONY : clean
>> +clean:
>> +    $(RM) $(LIB) $(OBJ) $(TEST)
>> diff --git a/README.md b/README.md
>> index 960468d..97b37df 100644
>> --- a/README.md
>> +++ b/README.md
>> @@ -1,14 +1,154 @@
>>   # libresource
>> +library of interfaces through which we can get system resource 
>> information
>> +like memory, CPU, stat, networking, device etc.
>> +Currently most of such information is read from /proc and /sys.
>>   -libresource is a community effort to generate a new library which 
>> tools
>> -like ps, top, etc can use to get information typically garnered from
>> -/proc.  libresource however will attempt to virtualize the data 
>> according
>> -to the calling task's cgroup information.
>> +Use case:
>> +1: Ease of use -
>> +Currently applications and tools need to read this info mostly from
>> +/proc and /sys file-systems. In most of the cases complex string
>> +parsing is involved which is needed to be done in application
>> +code. With the library interfaces application can get the information
>> +directly and all the string parsing, if any, will be done by library.
>> +2: Stability -
>> +If the format in which the information is provided in /proc or /sys
>> +file-system is changed then the application code is changed to align
>> +with those changes.
>
> What is the method by which you do this? Is there a chance the library 
> breaks when /proc or /sys is updated, until libresource is updated?

If something changes in /proc which changes the format, then libresource 
code must need to change to pick those changes, else libresource 
interfaces will give ENODATA or such error till libresource is update. 
If the changes are done taking care of existing format, for example 
appending new info at the end of proc file, then libresource will keep 
working,


Basically the idea is that application will not need to make any code 
changes, administrator just need to update the library and it should 
work. And by making code changes to library, all applications using the 
library will start using updated information. However I understand that, 
libresource should pick such changes as soon as it is available in /proc 
(or any such file) for it to work.

Also there is a library version which can be checked by applications to 
make decisions.

With that, if there are any significant changes to a particular resource 
then resource ID will be suffixed with V1, V2 etc. Old version will also 
work in that case for backward compatibility.

>
>  Also if a better way to get information comes in
>> +future, like through a syscall or a sysconf then again application code
>> +needs to be changed to get the benefit of it. Library will take care of
>> +such changes.
>> +3: Virtualization -
>> +In cases where DB is running in a virtualized environment using
>> +cgroup or namespaces, reading from /proc and /sys file-systems
>> +might not give correct information as these are not cgroup
>> +aware. Library API will take care of this e.g. if a process
>> +is running in a cgroup then library should provide information
>> +which is local to that cgroup.
>>   -# contributing
>> +Design:
>> +Each resource is identified by a resource id. User land application 
>> should
>> +provide the id while calling the interfaces to fetch the information.
>> +Following are resource IDs which are already implemented.
>>   -We will interact through pull requests and the issue tracker.  I 
>> think we
>> -should start with pull requests editing the CONTRIBUTORS so we can 
>> have a
>> -feel whether enough people are able to contribute or interested in 
>> following
>> -for the project to be successful.  We can also start pull requests 
>> against
>> -the specs/api.md file to discuss what that should look like.
>> +RES_MEM_ACTIVE
>> +RES_MEM_INACTIVE
>> +RES_MEM_AVAILABLE
>> +RES_MEM_FREE
>> +RES_MEM_TOTAL
>> +RES_MEM_PAGESIZE
>> +RES_MEM_SWAPFREE
>> +RES_MEM_SWAPTOTAL
>> +RES_KERN_COMPILE_TIME
>> +RES_KERN_RELEASE
>> +RES_NET_ALLIFSTAT
>> +RES_NET_IFSTAT
>> +RES_MEM_INFOALL
>> +
>> +Interfaces:
>> +1:
>> +int res_read(int res_id, void *out, void *hint, int pid, int flags); 
>> > +
>> +This is to read a resource information. A valid resource id should 
>> be provided
>> +in res_id, out should be properly allocated on the basis of size of 
>> resource
>> +information, hint should be given where needed. Currently pid and 
>> flags are
>> +not used, they are for future extensions.
>> +
>> +2:
>> +
>> +If an application wants to read multiple resource information in one 
>> call, it
>> +can call res_*_blk APIs to do so.
>> +
>> +2.1 structs
>> +Following struct holds information about one resource in such case.
>> +typedef struct res_unit {
>> +        int status;
>> +        int res_id;
>> +        void *hint;
>> +        union r_data data;
>> +} res_unit_t;
>> +
>> +An array of these form res_blk_t structure as below.
>> +typedef struct res_blk {
>> +        int res_count;
>> +        res_unit_t *res_unit[0];
>> +} res_blk_t;
>> +
>> +res_blk_t strcut is used in all res_*_blk interfaces.
>
> s/strcut/struct
Will spell check again on whole PATCH.
>
>> +
>> +2.2 functions
>> +res_blk_t *res_build_blk(int *res_ids, int res_count);
>> +
>> +This allocates memory for resources and initiates them properly. 
>> res_ids
>> +holds and array of valid resource ids and res_count holds number of
>> +resource ids. It also initiates struct fields properly.
>> +
>> +int res_read_blk(res_blk_t *resblk, int pid, int flags);
>> +Reading bulk resource information. Memory must be properly allocated 
>> and
>> +all fields should be properly filled to return error free resource
>> +information. res_build_blk call is suggested to allocate build 
>> res_blk_t
>> +structure.
>
> It seems that flags, since for future extensions, would be per 
> res_unit (like the hint). Could you explain why flags is per res_blk?
I was thinking flags as clues for overall libresource interfaces instead 
of individual resource ids.
For example we can put a "consistent single snapshot"bit in the flag, 
which will mean that
all information fetched should show one snapshot of system resource i.e. 
for example
memfree, memtoal, and memavailble should be read at once instead of one
by one where values might change in middle of reads.

For that reason they are one per interface.
>
>> +
>> +/* Free allocated memory from res_build_blk */
>> +extern void res_destroy_blk(res_blk_t *resblk);
>> +
>> +Example
>> +1:
>> +Reading total memory
>> +size_t stemp = 0;
>> +res_read(RES_MEM_TOTAL,&stemp,NULL, 0, 0);
>> +printf("MEMTOTAL is: %zu\n", stemp);
>> +
>> +
>> +2:
>> +reading network interface related statistics for interface named "lo"
>> +res_net_ifstat_t ifstat;
>> +res_read(RES_NET_IFSTAT,&ifstat, (void *)"lo",0, 0);
>> +printf("status for %s: %llu %llru\n", ifstat.ifname,
>> +    ifstat.rx_bytes,
>> +    ifstat.rx_packets
>> +    );
>> +3:
>> +Reading multiple resouce information in one call.
>
> s/resouce/resource
>
>> +
>> +    res_blk_t *b = NULL;
>> +    int a[NUM] = {RES_MEM_PAGESIZE,
>> +                RES_MEM_TOTAL,
>> +                RES_MEM_AVAILABLE,
>> +                RES_MEM_INFOALL,
>> +                RES_KERN_RELEASE,
>> +                RES_NET_IFSTAT,
>> +                RES_NET_ALLIFSTAT,
>> +                RES_KERN_COMPILE_TIME
>> +                };
>> +    b = res_build_blk(a, NUM);
>> +    b->res_unit[5]->hint = (void *)"lo";
>> +
>> +    res_read_blk(b, 0, 0);
>> +
>> +    printf("pagesize %ld bytes,\n memtotal %ld kb,\n memavailable 
>> %ld kb,\n"
>> +            " mefree %ld kb,\n release %s,\n compile time %s\n",
>
> s/mefree/memfree
>
>> + b->res_unit[0]->data.sz,
>> +            b->res_unit[1]->data.sz,
>> +            b->res_unit[2]->data.sz,
>> +            ((res_mem_infoall_t *)(b->res_unit[3]->data.ptr))->memfree,
>> +            b->res_unit[4]->data.str,
>> +            b->res_unit[7]->data.str
>> +    );
>> +
>> +    res_net_ifstat_t *ip = (res_net_ifstat_t 
>> *)b->res_unit[5]->data.ptr;
>> +    printf("stat for interface %s: %llu %llu\n", ip->ifname,
>> +        ip->rx_bytes,
>> +        ip->rx_packets
>> +        );
>> +
>> +    int k = (int)(long long)b->res_unit[6]->hint;
>> +    res_net_ifstat_t *ipp = (res_net_ifstat_t 
>> *)b->res_unit[6]->data.ptr;
>> +    for (int j=0; j< k; j++) {
>> +        printf("stat for interface %s: %llu %llu\n", ipp[j].ifname,
>> +            ipp[j].rx_bytes,
>> +            ipp[j].rx_packets
>> +        );
>> +    }
>> +
>> +    free(ipp);
>
> What is ipp?
For some resources (very few) memory might be allocated at the time of 
read, for such resources
it needs to be freed in application code.

I shall provide such info in man page, I am trying to figure out how to 
add man page for libraries.
For now I will update README with such info.

>
>> +    res_destroy_blk(b);
>
>
>> <snip>
>
>
>> diff --git a/resource.h b/resource.h
>> new file mode 100644
>> index 0000000..963877d
>> --- /dev/null
>> +++ b/resource.h
>> @@ -0,0 +1,147 @@
>> +#ifndef    _RESOURCE_H
>> +#define    _RESOURCE_H
>> +
>> +#include <stdio.h>
>
> What is this file included for here?
It is not needed, will remove it.
>
>> +#include <net/if.h>
>> +
>> +/* libresource version */
>> +#define LIBRESOURCE_API_VERSION 1
>> +
>> +/* Possible status for libresource information returned */
>> +/* libresource information was fetched correctly */
>> +#define RES_STATUS_FILLED 0
>> +/* There was some error in fetching libresource information. In most 
>> cases
>> + * errno will be set.
>> + */
>> +#define RES_STATUS_EMPTY -1
>> +/* Resource information is not supported yet, or Invalid resource
>> + * information.
>
> Seems that notsupported should be differentiated from invalid resource 
> info.
For library any invalid resource_id number is same as not supported
resource_id numbers, right ? or I missed something here.

>
>> + */
>> +#define RES_STATUS_NOTSUPPORTED    -2
>> +/* If partial information was read for a libresource information. 
>> For example
>> + * a string was read partially.
>> + */
>> +#define RES_STATUS_TRUNCATED -3
>
> How do you know which information is valid? 
If status is FILLED that means information is valid.

> Is this if you want multiple units in a block and only some of the 
> units are filled?
Yes this status should be used in such cases .
>
>> +
>> +
>> +/* Maximum size of a resource information data type which can be 
>> returned
>> + * without explicitly allocating memory for it. If resource information
>> + * size is larger than this this a pointer to allocated memory will be
>> + * returned.
>> + */
>> +#define RES_UNIT_OUT_SIZE    256
>> +
>> +/* This union is used to return resource information of various 
>> types */
>> +union r_data {
>> +    int i;
>> +    size_t sz;
>> +    long l;
>> +    char str[RES_UNIT_OUT_SIZE];
>> +    void *ptr;
>> +};
>
> Hmm, this is a confusing idea, IMO. Maybe everything is in void *ptr? 
> And then this union could be collapsed.
Even if it is a pointer application will need to know what is the return 
type in order to cast it.
With pointer there will be one extra memory dereference to get the data, 
so i thought this
is faster.

>
>> +
>> +/* In case of res_read_blk, each resource information will be 
>> represented by
>> + * following structure.
>> + */
>> +typedef struct res_unit {
>> +    int status;
>> +    int res_id;
>> +    void *hint;
>> +    union r_data data;
>> +} res_unit_t;
>> +
>> +/* In case of bulk read (res_read_blk), this structure will hold all 
>> required
>> + * information needed to do so.
>> + */
>> +typedef struct res_blk {
>> +    int res_count;
>> +    res_unit_t *res_unit[0];
>> +} res_blk_t;
>> +
>> +/* Resource information is divided in broad categories and each
>> + * category is assigned a number range for its resource information
>> + * Memory related            (RES_MEM_*)        1024-
>> + * Network related            (RES_NET_*)        2048-
>> + * General kernel related        (RES_KERN_*)        3072-
>> + * This is done to facilitate any future optimization which can be made
>> + * on the basis of resource information (hashing etc ?)
>
> Should these be u64 instead of int then?
you are right, we can use unsigned in instead. Will do that.

>
>> + */
>> +#define MEM_MIN                1024
>> +#define RES_MEM_HUGEPAGEALL        1025
>> +#define RES_MEM_HUGEPAGESIZE        1026
>> +#define RES_MEM_INACTIVE        1027
>> +#define RES_MEM_INFOALL            1028
>> +#define RES_MEM_AVAILABLE        1029
>> +#define RES_MEM_FREE            1030
>> +#define RES_MEM_TOTAL            1031
>> +#define RES_MEM_PAGESIZE        1032
>> +#define RES_MEM_SHMALL            1033
>> +#define RES_MEM_SHMINFOALL        1034
>> +#define RES_MEM_SHMMAX            1035
>> +#define RES_MEM_SHMMNI            1036
>
> What is MNI?
My bad. It should be MIN, implementation for this is not in this patch 
yet, I will remove such #defines
which are not implemented.

>
>> +#define RES_MEM_SWAPFREE        1037
>> +#define RES_MEM_SWAPTOTAL        1038
>> +#define RES_MEM_ACTIVE            1039
>> +#define MEM_MAX                1040
>> +
>> +#define NET_MIN                2048
>> +#define RES_NET_IFSTAT            2049
>> +#define RES_NET_ALLIFSTAT        2050
>> +#define NET_MAX                2051
>> +
>> +#define KERN_MIN            3072
>> +#define RES_KERN_COMPILE_TIME        3073
>> +#define RES_KERN_RELEASE        3074
>> +#define KERN_MAX            3075
>> +
>> +/* Structure to return RES_MEM_INFOALL resource information */
>> +typedef struct res_mem_infoall {
>> +    size_t memfree;
>> +    size_t memtotal;
>> +    size_t memavailable;
>> +    size_t active;
>> +    size_t inactive;
>> +    size_t swaptotal;
>> +    size_t swapfree;
>> +} res_mem_infoall_t;
>> +
>> +/* Structure to return RES_MEM_ALLIFSTAT resource information */
>> +typedef struct res_net_ifstat {
>> +    char ifname[IFNAMSIZ];
>> +    unsigned long long rx_bytes;
>> +    unsigned long long rx_packets;
>> +    unsigned long rx_errors;
>> +    unsigned long rx_dropped;
>> +    unsigned long rx_fifo_err;
>> +    unsigned long rx_frame_err;
>> +    unsigned long rx_compressed;
>> +    unsigned long rx_multicast;
>> +    unsigned long long tx_bytes;
>> +    unsigned long long tx_packets;
>> +    unsigned long tx_errors;
>> +    unsigned long tx_dropped;
>> +    unsigned long tx_fifo_err;
>> +    unsigned long tx_collisions;
>> +    unsigned long tx_carrier_err;
>> +    unsigned long tx_compressed;
>> +} res_net_ifstat_t;
>
> Maybe this struct name should have the word all in it to be consistent 
> with struct res_mem_infoall?
For interfaces ifstat provides stat for one interface, allifstat might 
suggest that it is stat for all
interfaces. So I did not choose that name.

>
>> +
>> +/* Allocating memory and building a res_blk structure to return bulk
>> + * resource information.
>> + */
>> +extern res_blk_t *res_build_blk(int *res_ids, int res_count);
>> +
>> +/* Reading bulk resource information. Memory must be properly 
>> allocated and
>> + * all fields should be properly filled to return error free resource
>> + * information. res_build_blk call is suggested to allocate build 
>> res_blk_t
>
> s/allocate build/allocate and build
>
>> + * structure.
>> + */
>> +extern int res_read_blk(res_blk_t *resblk, int pid, int flags);
>> +
>> +/* Free allocated memory from res_build_blk */
>> +extern void res_destroy_blk(res_blk_t *resblk);
>> +
>> +/* Read a resource information. Memory for out should be properly 
>> allocated */
>> +extern int res_read(int res_id, void *out, void *hint, int pid, int 
>> flags);
>> +
>> +#endif /* RESOURCE_H */
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linuxcontainers.org/pipermail/libresource-devel/attachments/20180301/7cdffdd3/attachment-0001.html>


More information about the libresource-devel mailing list