Re: [PATCH v1 02/12] tmem: Retire XEN_SYSCTL_TMEM_OP_[SET_CAP|SAVE_GET_CLIENT_CAP]

[Konrad Rzeszutek Wilk <konrad@kernel.org>]

On Wed, Sep 28, 2016 at 06:10:58AM -0600, Jan Beulich wrote:
> >>> On 28.09.16 at 11:42, <konrad.wilk@oracle.com> wrote:
> > It is not used by anything.
> 
> But that shouldn't be the only aspect. Are they also not useful for
> anything?

As far as I can see it was meant to complement the 'weight'. But the
code just hangs around. I can re-introduce it if there is a need for it
and make it visible via the  XEN_SYSCTL_TMEM_SET_CLIENT_INFO (introduced
in patch #7).

> 
> > --- a/xen/common/tmem_control.c
> > +++ b/xen/common/tmem_control.c
> > @@ -103,9 +103,9 @@ static int tmemc_list_client(struct client *c, tmem_cli_va_param_t buf,
> >      struct tmem_pool *p;
> >      bool_t s;
> >  
> > -    n = scnprintf(info,BSIZE,"C=CI:%d,ww:%d,ca:%d,co:%d,fr:%d,"
> > +    n = scnprintf(info,BSIZE,"C=CI:%d,ww:%d,co:%d,fr:%d,"
> >          "Tc:%"PRIu64",Ge:%ld,Pp:%ld,Gp:%ld%c",
> > -        c->cli_id, c->weight, c->cap, c->compress, c->frozen,
> > +        c->cli_id, c->weight, c->compress, c->frozen,
> >          c->total_cycles, c->succ_eph_gets, c->succ_pers_puts, c->succ_pers_gets,
> >          use_long ? ',' : '\n');
> >      if (use_long)
> > @@ -273,11 +273,6 @@ static int __tmemc_set_var(struct client *client, uint32_t subop, uint32_t arg1)
> >          atomic_sub(old_weight,&tmem_global.client_weight_total);
> >          atomic_add(client->weight,&tmem_global.client_weight_total);
> >          break;
> > -    case XEN_SYSCTL_TMEM_OP_SET_CAP:
> > -        client->cap = arg1;
> > -        tmem_client_info("tmem: cap set to %d for %s=%d\n",
> > -                        arg1, tmem_cli_id_str, cli_id);
> > -        break;
> >      case XEN_SYSCTL_TMEM_OP_SET_COMPRESS:
> >          if ( tmem_dedup_enabled() )
> >          {
> > @@ -341,11 +336,6 @@ static int tmemc_save_subop(int cli_id, uint32_t pool_id,
> >              break;
> >          rc = client->weight == -1 ? -2 : client->weight;
> >          break;
> > -    case XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_CAP:
> > -        if ( client == NULL )
> > -            break;
> > -        rc = client->cap == -1 ? -2 : client->cap;
> > -        break;
> >      case XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_FLAGS:
> >          if ( client == NULL )
> >              break;
> 
> It looks like you're removing all accesses to the cap field. That would
> suggest that you now want to also remove the field itself.

Yes! The patch titled "06/12] tmem: Move client
weight,frozen,live_migrating, and compress" did it, but it should have
been done here. Thanks!
> 
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -757,14 +757,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_psr_cat_op_t);
> >  #define XEN_SYSCTL_TMEM_OP_DESTROY                3
> >  #define XEN_SYSCTL_TMEM_OP_LIST                   4
> >  #define XEN_SYSCTL_TMEM_OP_SET_WEIGHT             5
> > -#define XEN_SYSCTL_TMEM_OP_SET_CAP                6
> >  #define XEN_SYSCTL_TMEM_OP_SET_COMPRESS           7
> >  #define XEN_SYSCTL_TMEM_OP_QUERY_FREEABLE_MB      8
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_BEGIN             10
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_GET_VERSION       11
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_GET_MAXPOOLS      12
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_WEIGHT 13
> > -#define XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_CAP    14
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_GET_CLIENT_FLAGS  15
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_FLAGS    16
> >  #define XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_NPAGES   17
> 
> I think such removals should be accompanied by bumping
> XEN_SYSCTL_INTERFACE_VERSION, albeit it's obviously not as
> relevant as it would be when changing some structure's layout.

The patch series actually does not alter the layout per say. I think
it would be most justified in "11/12] tmem/xc_tmem_control: Rename
'arg1' to 'len' and 'arg2' to arg." as that:

"Of all the various sub-commands, the only one that needed
semantic change is XEN_SYSCTL_TMEM_OP_SAVE_BEGIN. That in the
past used 'arg1', and now we are moving it to use 'arg'."

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel