Re: [PATCH v4 13/13] tools/xenstore: switch quota management to be table based

[Juergen Gross <jgross@xxxxxxxx>]

On 03.05.23 12:53, Julien Grall wrote:
> Hi Juergen,
>
> On 05/04/2023 08:03, Juergen Gross wrote:
> > Instead of having individual quota variables switch to a table based
> > approach like the generic accounting. Include all the related data in
> > the same table and add accessor functions.
> >
> > This enables to use the command line --quota parameter for setting all
> > possible quota values, keeping the previous parameters for
> > compatibility.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V2:
> > - new patch
> > One further remark: it would be rather easy to add soft-quota for all
> > the other quotas (similar to the memory one). This could be used as
> > an early warning for the need to raise global quota.
> I don't have a strong opinion on this topic.
Me neither.

> > ---
> >   tools/xenstore/xenstored_control.c     |  43 ++------
> >   tools/xenstore/xenstored_core.c        |  85 ++++++++--------
> >   tools/xenstore/xenstored_core.h        |  10 --
> >   tools/xenstore/xenstored_domain.c      | 132 +++++++++++++++++--------
> >   tools/xenstore/xenstored_domain.h      |  12 ++-
> >   tools/xenstore/xenstored_transaction.c |   5 +-
> >   tools/xenstore/xenstored_watch.c       |   2 +-
> >   7 files changed, 155 insertions(+), 134 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_control.c 
> > b/tools/xenstore/xenstored_control.c
> > index a2ba64a15c..75f51a80db 100644
> > --- a/tools/xenstore/xenstored_control.c
> > +++ b/tools/xenstore/xenstored_control.c
> > @@ -221,35 +221,6 @@ static int do_control_log(const void *ctx, struct 
> > connection *conn,
> >       return 0;
> >   }
> > -struct quota {
> > -    const char *name;
> > -    int *quota;
> > -    const char *descr;
> > -};
> > -
> > -static const struct quota hard_quotas[] = {
> > -    { "nodes", &quota_nb_entry_per_domain, "Nodes per domain" },
> > -    { "watches", &quota_nb_watch_per_domain, "Watches per domain" },
> > -    { "transactions", &quota_max_transaction, "Transactions per domain" },
> > -    { "outstanding", &quota_req_outstanding,
> > -        "Outstanding requests per domain" },
> > -    { "transaction-nodes", &quota_trans_nodes,
> > -        "Max. number of accessed nodes per transaction" },
> > -    { "memory", &quota_memory_per_domain_hard,
> > -        "Total Xenstore memory per domain (error level)" },
> > -    { "node-size", &quota_max_entry_size, "Max. size of a node" },
> > -    { "path-max", &quota_max_path_len, "Max. length of a node path" },
> > -    { "permissions", &quota_nb_perms_per_node,
> > -        "Max. number of permissions per node" },
> > -    { NULL, NULL, NULL }
> > -};
> > -
> > -static const struct quota soft_quotas[] = {
> > -    { "memory", &quota_memory_per_domain_soft,
> > -        "Total Xenstore memory per domain (warning level)" },
> > -    { NULL, NULL, NULL }
> > -};
> > -
> >   static int quota_show_current(const void *ctx, struct connection *conn,
> >                     const struct quota *quotas)
> >   {
> > @@ -260,9 +231,11 @@ static int quota_show_current(const void *ctx, struct 
> > connection *conn,
> >       if (!resp)
> >           return ENOMEM;
> > -    for (i = 0; quotas[i].quota; i++) {
> > +    for (i = 0; i < ACC_N; i++) {
> > +        if (!quotas[i].name)
> > +            continue;
> >           resp = talloc_asprintf_append(resp, "%-17s: %8d %s\n",
> > -                          quotas[i].name, *quotas[i].quota,
> > +                          quotas[i].name, quotas[i].val,
> >                             quotas[i].descr);
> >           if (!resp)
> >               return ENOMEM;
> > @@ -274,7 +247,7 @@ static int quota_show_current(const void *ctx, struct 
> > connection *conn,
> >   }
> >   static int quota_set(const void *ctx, struct connection *conn,
> > -             char **vec, int num, const struct quota *quotas)
> > +             char **vec, int num, struct quota *quotas)
> >   {
> >       unsigned int i;
> >       int val;
> > @@ -286,9 +259,9 @@ static int quota_set(const void *ctx, struct connection 
> > *conn,
> >       if (val < 1)
> >           return EINVAL;
> > -    for (i = 0; quotas[i].quota; i++) {
> > -        if (!strcmp(vec[0], quotas[i].name)) {
> > -            *quotas[i].quota = val;
> > +    for (i = 0; i < ACC_N; i++) {
> > +        if (quotas[i].name && !strcmp(vec[0], quotas[i].name)) {
> > +            quotas[i].val = val;
> >               send_ack(conn, XS_CONTROL);
> >               return 0;
> >           }
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index 65df2866bf..6e2fc06840 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -89,17 +89,6 @@ unsigned int trace_flags = TRACE_OBJ | TRACE_IO;
> >   static const char *sockmsg_string(enum xsd_sockmsg_type type);
> > -int quota_nb_entry_per_domain = 1000;
> > -int quota_nb_watch_per_domain = 128;
> > -int quota_max_entry_size = 2048; /* 2K */
> > -int quota_max_transaction = 10;
> > -int quota_nb_perms_per_node = 5;
> > -int quota_trans_nodes = 1024;
> > -int quota_max_path_len = XENSTORE_REL_PATH_MAX;
> > -int quota_req_outstanding = 20;
> > -int quota_memory_per_domain_soft = 2 * 1024 * 1024; /* 2 MB */
> > -int quota_memory_per_domain_hard = 2 * 1024 * 1024 + 512 * 1024; /* 2.5 MB */
> > -
> >   unsigned int timeout_watch_event_msec = 20000;
> >   void trace(const char *fmt, ...)
> > @@ -799,7 +788,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, 
> > struct node *node,
> >           + node->perms.num * sizeof(node->perms.p[0])
> >           + node->datalen + node->childlen;
> > -    if (domain_max_chk(conn, ACC_NODESZ, data.dsize, quota_max_entry_size)
> > +    if (domain_max_chk(conn, ACC_NODESZ, data.dsize)
> >           && !no_quota_check) {
> >           errno = ENOSPC;
> >           return errno;
> > @@ -1188,8 +1177,7 @@ bool is_valid_nodename(const struct connection *conn, 
> > const char *node)
> >       if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
> >           local_off = 0;
> > -    if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off,
> > -               quota_max_path_len))
> > +    if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off))
> >           return false;
> >       return valid_chars(node);
> > @@ -1501,7 +1489,7 @@ static struct node *create_node(struct connection *conn, 
> > const void *ctx,
> >       for (i = node; i; i = i->parent) {
> >           /* i->parent is set for each new node, so check quota. */
> >           if (i->parent &&
> > -            domain_nbentry(conn) >= quota_nb_entry_per_domain) {
> > +            domain_nbentry(conn) >= hard_quotas[ACC_NODES].val) {
> >               ret = ENOSPC;
> >               goto err;
> >           }
> > @@ -1776,7 +1764,7 @@ static int do_set_perms(const void *ctx, struct 
> > connection *conn,
> >           return EINVAL;
> >       perms.num--;
> > -    if (domain_max_chk(conn, ACC_NPERM, perms.num, quota_nb_perms_per_node))
> > +    if (domain_max_chk(conn, ACC_NPERM, perms.num))
> >           return ENOSPC;
> >       permstr = in->buffer + strlen(in->buffer) + 1;
> > @@ -2644,7 +2632,16 @@ static void usage(void)
> >   "                          memory: total used memory per domain for nodes,\n"
> >   "                                  transactions, watches and requests, 
> > above\n"
> >   "                                  which Xenstore will stop talking to 
> > domain\n"
> > +"                          nodes: number nodes owned by a domain\n"
> > +"                          node-permissions: number of access permissions 
> > per\n"
> > +"                                            node\n"
> > +"                          node-size: total size of a node (permissions +\n"
> > +"                                     children names + content)\n"
> >   "                          outstanding: number of outstanding requests\n"
> > +"                          path-length: length of a node path\n"
> > +"                          transactions: number of concurrent transactions\n"
> > +"                                        per domain\n"
> > +"                          watches: number of watches per domain"
> >   "  -q, --quota-soft <what>=<nb> set a soft quota <what> to the value <nb>,\n"
> >   "                          causing a warning to be issued via syslog() if 
> > the\n"
> >   "                          limit is violated, allowed quotas are:\n"
> > @@ -2695,12 +2692,12 @@ int dom0_domid = 0;
> >   int dom0_event = 0;
> >   int priv_domid = 0;
> > -static int get_optval_int(const char *arg)
> > +static unsigned int get_optval_int(const char *arg)
> >   {
> >       char *end;
> > -    long val;
> > +    unsigned long val;
> > -    val = strtol(arg, &end, 10);
> > +    val = strtoul(arg, &end, 10);
> The changes in get_optval_int() feels like more a clean-up because the returned 
> value cannot be negative (see check below). I would prefer if they are done in a 
> separate patch.
Okay.

> >       if (!*arg || *end || val < 0 || val > INT_MAX)
> Now that 'val' is unsigned long, then there is no point for checking val is < 0.
Oh, indeed.

> Lastly, I would rename the helper to make clear it returns an unsigned value. 
> How about get_optval_uint()?
Okay.

> >           barf("invalid parameter value \"%s\"\n", arg);
> > @@ -2709,15 +2706,19 @@ static int get_optval_int(const char *arg)
> >   static bool what_matches(const char *arg, const char *what)
> >   {
> > -    unsigned int what_len = strlen(what);
> > +    unsigned int what_len;
> > +
> > +    if (!what)
> > +        false;
> Shouldn't this be "return false"?
Yes.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature