Re: [Xen-devel] [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

> On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
>> tools/blktap2/drivers/Makefile        |   2 +-
>>  tools/blktap2/drivers/tapdisk-image.c |   2 +-
>>  tools/blktap2/drivers/tapdisk-vbd.c   |   6 +++---
>>  tools/blktap2/drivers/tapdisk.h       |   6 +++++-
>>  tools/memshr/bidir-daemon.c           |  34
>> ++++++++++++++++++++++++----------
>>  tools/memshr/bidir-hash.h             |  13 ++++++++-----
>>  tools/memshr/interface.c              |  30
>> ++++++++++++++++++------------
>>  tools/memshr/memshr.h                 |  11 +++++++++--
>>  8 files changed, 69 insertions(+), 35 deletions(-)
>>
>>
>> The only (in-tree, that we know of) consumer of the mem sharing API
>> is the memshr tool (conditionally linked into blktap2). Update it to
>> use the new API.
>
> At least some of this must happen in the previous 2 patches so that the
> build is not broken.
>
> Is there any benefit to conditionally linking this stuff? Is it lots of
> code or does it have some other undesirable impact when built even when
> it is quiescent? Seems like a recipe for bit rot to not build it, plus
> it makes it harder for people to consume via distro packages etc.

Let me clarify this a bit: our interest in memshr is in not breaking it
with our changes. We don't know why it's built that way. It was like that
wen we arrived. We don't change that. We certainly don't intend to own it.

I would frame line 44 at
http://xenbits.xensource.com/hg/xen-unstable.hg/file/1c58bb664d8d/tools/blktap2/drivers/Makefile
as the definition of bit rot

Andres

>
>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>>
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/Makefile
>> --- a/tools/blktap2/drivers/Makefile
>> +++ b/tools/blktap2/drivers/Makefile
>> @@ -43,7 +43,7 @@ MEMSHR_DIR = $(XEN_ROOT)/tools/memshr
>>  MEMSHRLIBS :=
>>  ifeq ($(CONFIG_Linux), __fixme__)
>>  CFLAGS += -DMEMSHR
>> -MEMSHRLIBS += $(MEMSHR_DIR)/libmemshr.a
>> +MEMSHRLIBS += -L$(XEN_ROOT)/tools/libxc -lxenctrl
>> $(MEMSHR_DIR)/libmemshr.a
>
> Please use $(LDLIBS_libxenctrl).
>
> Is there a reason libmemshr is not a shared library?
>
>>  endif
>>
>>  ifeq ($(VHD_STATIC),y)
>> diff -r 892049dfc1c9 -r e16f5d2643c9
>> tools/blktap2/drivers/tapdisk-image.c
>> --- a/tools/blktap2/drivers/tapdisk-image.c
>> +++ b/tools/blktap2/drivers/tapdisk-image.c
>> @@ -60,7 +60,7 @@ tapdisk_image_allocate(const char *file,
>>         image->storage   = storage;
>>         image->private   = private;
>>  #ifdef MEMSHR
>> -       image->memshr_id = memshr_vbd_image_get(file);
>> +       image->memshr_id = memshr_vbd_image_get((char *)file);
>
> Casting away the const here raises a big red flag. Why is this
> necessary? You should either fix the API of memshr_vbd_image_get or
> tapdisk_image_allocate IMHO and propagate the change as necessary.
> AFAICT there is no reason not to push the const'ness down into the
> memshr API.
>
>>  #endif
>>         INIT_LIST_HEAD(&image->next);
>>
> [...]
>
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/bidir-hash.h
>> --- a/tools/memshr/bidir-hash.h
>> +++ b/tools/memshr/bidir-hash.h
>> @@ -81,15 +81,16 @@ static int fgprtshr_mfn_cmp(uint32_t m1,
>>  #undef BIDIR_VALUE
>>  #undef BIDIR_KEY_T
>>  #undef BIDIR_VALUE_T
>> +
>>  /* TODO better hashes! */
>>  static inline uint32_t blockshr_block_hash(vbdblk_t block)
>>  {
>>      return (uint32_t)(block.sec) ^ (uint32_t)(block.disk_id);
>>  }
>>
>> -static inline uint32_t blockshr_shrhnd_hash(uint64_t shrhnd)
>> +static inline uint32_t blockshr_shrhnd_hash(share_tuple_t shrhnd)
>>  {
>> -    return (uint32_t)shrhnd;
>> +    return ((uint32_t) shrhnd.handle);
>>  }
>>
>>  static inline int blockshr_block_cmp(vbdblk_t b1, vbdblk_t b2)
>> @@ -97,15 +98,17 @@ static inline int blockshr_block_cmp(vbd
>>      return (b1.sec == b2.sec) && (b1.disk_id == b2.disk_id);
>>  }
>>
>> -static inline int blockshr_shrhnd_cmp(uint64_t h1, uint64_t h2)
>> +static inline int blockshr_shrhnd_cmp(share_tuple_t h1, share_tuple_t
>> h2)
>>  {
>> -    return (h1 == h2);
>> +    return ( (h1.domain == h2.domain) &&
>> +             (h1.frame  == h2.frame) &&
>> +             (h1.handle == h2.handle) );
>
> memcmp?
>
>>  }
>>  #define BIDIR_NAME_PREFIX       blockshr
>>  #define BIDIR_KEY               block
>>  #define BIDIR_VALUE             shrhnd
>>  #define BIDIR_KEY_T             vbdblk_t
>> -#define BIDIR_VALUE_T           uint64_t
>> +#define BIDIR_VALUE_T           share_tuple_t
>>  #include "bidir-namedefs.h"
>>
>>  #endif /* BLOCK_MAP */
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/interface.c
>> --- a/tools/memshr/interface.c
>> +++ b/tools/memshr/interface.c
>> @@ -145,16 +145,17 @@ void memshr_vbd_image_put(uint16_t memsh
>>
>>  int memshr_vbd_issue_ro_request(char *buf,
>>                                  grant_ref_t gref,
>> -                                uint16_t file_id,
>> +                                uint16_t file_id,
>>                                  uint64_t sec,
>>                                  int secs,
>> -                                uint64_t *hnd)
>> +                                share_tuple_t *hnd)
>>  {
>>      vbdblk_t blk;
>> -    uint64_t s_hnd, c_hnd;
>> +    share_tuple_t source_st, client_st;
>> +    uint64_t c_hnd;
>>      int ret;
>>
>> -    *hnd = 0;
>> +    *hnd = (share_tuple_t){ 0, 0, 0 };
>>      if(!vbd_info.enabled)
>>          return -1;
>>
>> @@ -169,26 +170,31 @@ int memshr_vbd_issue_ro_request(char *bu
>>      /* If page couldn't be made sharable, we cannot do anything about
>> it */
>>      if(ret != 0)
>>          return -3;
>> -    *hnd = c_hnd;
>> +
>> +    *(&client_st) = (share_tuple_t){ vbd_info.domid, gref, c_hnd };
>
> Is "*(&thing)" somehow different to "thing"?
>
>> +    *hnd = client_st;
>>
>>      /* Check if we've read matching disk block previously */
>>      blk.sec     = sec;
>>      blk.disk_id = file_id;
>> -    if(blockshr_block_lookup(memshr.blks, blk, &s_hnd) > 0)
>> +    if(blockshr_block_lookup(memshr.blks, blk, &source_st) > 0)
>>      {
>> -        ret = xc_memshr_share(vbd_info.xc_handle, s_hnd, c_hnd);
>> +        ret = xc_memshr_share(vbd_info.xc_handle, source_st.domain,
>> source_st.frame,
>> +                                source_st.handle, 1, vbd_info.domid,
>> gref, c_hnd, 1);
>
> These 1's == is a gref but I only know that because I was just reading
> that patch. If you decide you do need to keep the flag separate then
> this would be better as a flags argument with named values
> XC_MEMSHR_SOURCE_IS_GREF etc.
>
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/memshr.h
>> --- a/tools/memshr/memshr.h
>> +++ b/tools/memshr/memshr.h
>> @@ -25,6 +25,13 @@
>>
>>  typedef uint64_t xen_mfn_t;
>>
>> +typedef struct share_tuple
>> +{
>> +    uint32_t domain;
>> +    uint64_t frame;
>> +    uint64_t handle;
>> +} share_tuple_t;
>
> Didn't you just add these as three separate parameters to
> xc_memshr_share? Seems like this could be xc_memshr_tuple (or some
> better name with more meaning than "tuple") and used throughout.
>
> Ian.
>
>
>



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel