Re: [PATCH resend V10 04/12] Resctrl: Add private interfaces to operate cache bank

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Mon, Mar 06, 2017 at 06:06:33PM +0800, Eli Qiao wrote:
> virResCtrlSetCacheBanks: Set cache banks of a libvirt domain. It will
>                         create new resource domain under
>                         `/sys/fs/resctrl` and fill the schemata according
>                         the cache bank configration.
>
> virResCtrlUpdate: Destroy resctrl domain directory, and update the schemata
>                  after libvirt domain destroy.
>
> Signed-off-by: Eli Qiao <liyong.qiao@xxxxxxxxx>
> ---
> src/libvirt_private.syms |   6 +-
> src/util/virresctrl.c    | 677 +++++++++++++++++++++++++++++++++++++++++++++++
> src/util/virresctrl.h    |   5 +-
> 3 files changed, 686 insertions(+), 2 deletions(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index b8445ef..9cfffb8 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2325,7 +2325,11 @@ virRandomInt;
> virResCtrlAvailable;
> virResCtrlGet;
> virResCtrlInit;
> -

Don't delete that line, keep two empty lines so that the file is
consistent.  Every time you do something somewhere in some file, look
around the file and try keeping stuff consistent.

> +virResCtrlSetCacheBanks;
> +virResCtrlTypeFromString;
> +virResCtrlTypeToString;
> +virResCtrlUpdate;
> +#
>
> # util/virrotatingfile.h
> virRotatingFileReaderConsume;
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index eee6675..43af0f5 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -19,6 +19,8 @@
>  *  Eli Qiao <liyong.qiao@xxxxxxxxx>
>  */
> #include <config.h>
> +#include <fcntl.h>
> +#include <sys/stat.h>
>
> #include "virresctrl.h"
> #include "viralloc.h"
> @@ -61,8 +63,58 @@ do { \
>
> #define VIR_RESCTRL_GET_SCHEMATA(count) ((1 << count) - 1)
>
> +/**
> + * a virResSchemata represents a schemata object under a resource control
> + * domain.
> + */
> +typedef struct _virResSchemataItem virResSchemataItem;
> +typedef virResSchemataItem *virResSchemataItemPtr;
> +struct _virResSchemataItem {
> +    unsigned int socket_no;
> +    unsigned schemata;
> +};
> +
> +typedef struct _virResSchemata virResSchemata;
> +typedef virResSchemata *virResSchemataPtr;
> +struct _virResSchemata {
> +    unsigned int n_schemata_items;
> +    virResSchemataItemPtr schemata_items;
> +};
> +
> +/**
> + * a virResDomain represents a resource control domain. It's a double linked
> + * list.
> + */
> +
> +typedef struct _virResDomain virResDomain;
> +typedef virResDomain *virResDomainPtr;
> +
> +struct _virResDomain {
> +    char *name;
> +    virResSchemataPtr schematas[VIR_RDT_RESOURCE_LAST];
> +    char **tasks;
> +    size_t n_tasks;
> +    size_t n_sockets;
> +    virResDomainPtr pre;
> +    virResDomainPtr next;
> +};
> +
> +/* All resource control domains on this host*/
> +typedef struct _virResCtrlDomain virResCtrlDomain;
> +typedef virResCtrlDomain *virResCtrlDomainPtr;
> +
> +struct _virResCtrlDomain {
> +    unsigned int num_domains;
> +    virResDomainPtr domains;
> +};
> +
> static unsigned int host_id;
>
> +/* Global static struct to be maintained which is a interface */
> +static virResCtrlDomain domainall;
> +
> +/* Global static struct array to be maintained which indicate
> + * resource status on a host */
> static virResCtrl resctrlall[] = {
>     {
>         .name = "L3",
> @@ -82,6 +134,78 @@ static virResCtrl resctrlall[] = {
>     },
> };
>
> +/*
> + * How many bits is set in schemata
> + * eg:
> + * virResCtrlBitsNum(10110) = 2 */

You mean how many consecutive bits?  Why are you not using (and
extending in case there's something missing) the virBitmap for this kind
of things?  Also going bit by bit in such a big loop, I doubt compiler
will optimize this for us.  Can we do something like:

 ffs((schemata >> ffs(schemata)) + 1)

instead?

> +static int virResCtrlBitsContinuesNum(unsigned schemata)
> +{
> +    size_t i;
> +    int ret = 0;
> +    for (i = 0; i < MAX_CBM_BIT_LEN; i ++) {
> +        if ((schemata & 0x1) == 0x1)
> +            ret++;
> +        else
> +            if (ret > 0 || schemata == 0) break;
> +
> +        schemata = schemata >> 1;
> +    }
> +    return ret;
> +}
> +
> +/* Position of the highest continue 1 bit of in schemata
> + * eg:
> + * virResctrlBitsContinuesPos(10110) = 3 */
> +static int virResCtrlBitsContinuesPos(unsigned schemata)

full type names please (unsigned int)

> +{
> +    size_t i;
> +    int flag = 0;
> +    for (i = 0; i < MAX_CBM_BIT_LEN; i ++) {
> +        if ((schemata & 0x1) == 0x0 && flag == 1)
> +            return i;
> +        else if ((schemata & 0x1) == 0x1) flag = 1;
> +
> +        schemata = schemata >> 1;
> +    }
> +    return 0;

something like:

  f = ffs(schemata)
  return f + (schemata >> f) + 1

???

[...]

> +static int virResCtrlGetSchemata(const int type, const char *name, char **schemata)
> +{
> +    int rc;
> +    char *tmp, *end;
> +    char *buf;
> +
> +    if ((rc = virResCtrlGetStr(name, "schemata", &buf)) < 0)
> +        return rc;
> +
> +    tmp = strstr(buf, resctrlall[type].name);

You will find L3CODE when looking for L3 as well.

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list