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

Re: [Xen-devel] [PATCH v2 5/8] tmem: Move TMEM_CONTROL subop of tmem hypercall to sysctl.



On 27/08/15 19:47, Konrad Rzeszutek Wilk wrote:
>
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -710,6 +710,48 @@ struct xen_sysctl_psr_cat_op {
>>>  typedef struct xen_sysctl_psr_cat_op xen_sysctl_psr_cat_op_t;
>>>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_psr_cat_op_t);
>>>  
>>> +#define XEN_SYSCTL_TMEM_OP_ALL_CLIENTS 0xFFFFU
>>> +
>>> +#define XEN_SYSCTL_TMEM_OP_THAW                   0
>>> +#define XEN_SYSCTL_TMEM_OP_FREEZE                 1
>>> +#define XEN_SYSCTL_TMEM_OP_FLUSH                  2
>>> +#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
>>> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_POOL_UUID     18
>>> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_PAGE     19
>>> +#define XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_INV      20
>>> +#define XEN_SYSCTL_TMEM_OP_SAVE_END               21
>>> +#define XEN_SYSCTL_TMEM_OP_RESTORE_BEGIN          30
>>> +#define XEN_SYSCTL_TMEM_OP_RESTORE_PUT_PAGE       32
>>> +#define XEN_SYSCTL_TMEM_OP_RESTORE_FLUSH_PAGE     33
>>> +
>>> +struct xen_sysctl_tmem_op {
>>> +    uint32_t cmd;       /* IN: XEN_SYSCTL_TMEM_OP_* . */
>>> +    int32_t pool_id;    /* IN: 0 by default unless _SAVE_*, RESTORE_* .*/
>>> +    uint32_t cli_id;    /* IN: client id, 0 for 
>>> XEN_SYSCTL_TMEM_QUERY_FREEABLE_MB
>>> +                           for all others can be the domain id or
>>> +                           XEN_SYSCTL_TMEM_OP_ALL_CLIENTS for all. */
>>> +    uint32_t arg1;      /* IN: If not applicable to command use 0. */
>>> +    uint32_t arg2;      /* IN: If not applicable to command use 0. */
>> Please can this interface be fixed as part of the move, even if it is in
>> subsequent patches for clarity.
> I will gladly fix this interface in further patches. By all means!

What I wish to avoid is 4.6 releasing with the API in this state, as
adjusting valgrind and strace to compensate will be miserable.

The best solution would be to have this patch and the fixups adjacent in
the series, at which point the valgrind/strace adjustments can start
with the clean API for 4.6

>>> +    uint64_t oid[3];    /* IN: If not applicable to command use 0. */
>>> +    XEN_GUEST_HANDLE_64(char) buf; /* IN/OUT: Buffer to save and restore 
>>> ops. */
>>> +};
>>> +typedef struct xen_sysctl_tmem_op xen_sysctl_tmem_op_t;
>>> +DEFINE_XEN_GUEST_HANDLE(xen_sysctl_tmem_op_t);
>>> +
>>>  struct xen_sysctl {
>>>      uint32_t cmd;
>>>  #define XEN_SYSCTL_readconsole                    1
>>> @@ -734,6 +776,7 @@ struct xen_sysctl {
>>>  #define XEN_SYSCTL_psr_cmt_op                    21
>>>  #define XEN_SYSCTL_pcitopoinfo                   22
>>>  #define XEN_SYSCTL_psr_cat_op                    23
>>> +#define XEN_SYSCTL_tmem_op                       24
>>>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>>>      union {
>>>          struct xen_sysctl_readconsole       readconsole;
>>> @@ -758,6 +801,7 @@ struct xen_sysctl {
>>>          struct xen_sysctl_coverage_op       coverage_op;
>>>          struct xen_sysctl_psr_cmt_op        psr_cmt_op;
>>>          struct xen_sysctl_psr_cat_op        psr_cat_op;
>>> +        struct xen_sysctl_tmem_op           tmem_op;
>>>          uint8_t                             pad[128];
>>>      } u;
>>>  };
>>> diff --git a/xen/include/public/tmem.h b/xen/include/public/tmem.h
>>> index 4fd2fc6..208f5a6 100644
>>> --- a/xen/include/public/tmem.h
>>> +++ b/xen/include/public/tmem.h
>>> @@ -33,7 +33,7 @@
>>>  #define TMEM_SPEC_VERSION          1
>>>  
>>>  /* Commands to HYPERVISOR_tmem_op() */
>>> -#define TMEM_CONTROL               0
>>> +#define TMEM_CONTROL_MOVED         0
>> If anything, this should be deleted as opposed to being renamed.  The
>> _MOVED suffix is confusing, as TMEM_CONTROL_MOVED sounds like it could
>> be a plausible TMEM operation.
> Ah, will do what Jan asked and just put a big comment. And keep the old
> name (or maybe add 'DEPRECATED'?)

If you are going to the effort of renaming, then just delete.  Either
way will break the build of unsuspecting code, and the latter leaves us
with less junk.

A comment however will be useful in all cases.

>>>  #define TMEM_NEW_POOL              1
>>>  #define TMEM_DESTROY_POOL          2
>>>  #define TMEM_PUT_PAGE              4
>>> @@ -48,35 +48,9 @@
>>>  #endif
>>>  
>>>  /* Privileged commands to HYPERVISOR_tmem_op() */
>>> -#define TMEM_AUTH                 101 
>>> +#define TMEM_AUTH                 101
>>>  #define TMEM_RESTORE_NEW          102
>>>  
>>> -/* Subops for HYPERVISOR_tmem_op(TMEM_CONTROL) */
>>> -#define TMEMC_THAW                   0
>>> -#define TMEMC_FREEZE                 1
>>> -#define TMEMC_FLUSH                  2
>>> -#define TMEMC_DESTROY                3
>>> -#define TMEMC_LIST                   4
>>> -#define TMEMC_SET_WEIGHT             5
>>> -#define TMEMC_SET_CAP                6
>>> -#define TMEMC_SET_COMPRESS           7
>>> -#define TMEMC_QUERY_FREEABLE_MB      8
>>> -#define TMEMC_SAVE_BEGIN             10
>>> -#define TMEMC_SAVE_GET_VERSION       11
>>> -#define TMEMC_SAVE_GET_MAXPOOLS      12
>>> -#define TMEMC_SAVE_GET_CLIENT_WEIGHT 13
>>> -#define TMEMC_SAVE_GET_CLIENT_CAP    14
>>> -#define TMEMC_SAVE_GET_CLIENT_FLAGS  15
>>> -#define TMEMC_SAVE_GET_POOL_FLAGS    16
>>> -#define TMEMC_SAVE_GET_POOL_NPAGES   17
>>> -#define TMEMC_SAVE_GET_POOL_UUID     18
>>> -#define TMEMC_SAVE_GET_NEXT_PAGE     19
>>> -#define TMEMC_SAVE_GET_NEXT_INV      20
>>> -#define TMEMC_SAVE_END               21
>>> -#define TMEMC_RESTORE_BEGIN          30
>>> -#define TMEMC_RESTORE_PUT_PAGE       32
>>> -#define TMEMC_RESTORE_FLUSH_PAGE     33
>>> -
>>>  /* Bits for HYPERVISOR_tmem_op(TMEM_NEW_POOL) */
>>>  #define TMEM_POOL_PERSIST          1
>>>  #define TMEM_POOL_SHARED           2
>>> @@ -110,16 +84,7 @@ struct tmem_op {
>>>              uint32_t flags;
>>>              uint32_t arg1;
>>>          } creat; /* for cmd == TMEM_NEW_POOL, TMEM_AUTH, TMEM_RESTORE_NEW 
>>> */
>>> -        struct { 
>>> -            uint32_t subop;
>>> -            uint32_t cli_id;
>>> -            uint32_t arg1;
>>> -            uint32_t arg2;
>>> -            uint64_t oid[3];
>>> -            tmem_cli_va_t buf;
>>> -        } ctrl; /* for cmd == TMEM_CONTROL */
>>>          struct {
>>> -            
>>>              uint64_t oid[3];
>>>              uint32_t index;
>>>              uint32_t tmem_offset;
>>> diff --git a/xen/include/xen/tmem.h b/xen/include/xen/tmem.h
>>> index 5dbf9d5..32a542a 100644
>>> --- a/xen/include/xen/tmem.h
>>> +++ b/xen/include/xen/tmem.h
>>> @@ -9,6 +9,9 @@
>>>  #ifndef __XEN_TMEM_H__
>>>  #define __XEN_TMEM_H__
>>>  
>>> +struct xen_sysctl_tmem_op;
>> #include<public/sysctl.h> please.
> I tried that and it blew up with tons of other dependencies. This makes
> it much simpler without adding extra #include.

Fair enough.

~Andrew

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.