Re: [Xen-devel] Xen-devel Digest, Vol 85, Issue 212

[zou <colin626262@xxxxxxxxx>]

to subscribe

xen-devel-request@xxxxxxxxxxxxxçåï

>Send Xen-devel mailing list submissions to
>       xen-devel@xxxxxxxxxxxxx
>
>To subscribe or unsubscribe via the World Wide Web, visit
>       http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel
>or, via email, send a message with subject or body 'help' to
>       xen-devel-request@xxxxxxxxxxxxx
>
>You can reach the person managing the list at
>       xen-devel-owner@xxxxxxxxxxxxx
>
>When replying, please edit your Subject line so it is more specific
>than "Re: Contents of Xen-devel digest..."
>
>
>Today's Topics:
>
>   1. Re: xl: expose max_cpu_id from `xl info` (Zhigang Wang)
>   2. Re: libxl and unsigned types (George Dunlap)
>   3. [PATCH] tools/blktap: reorder MEMSHR_DIR to fix CFLAGS
>      (Olaf Hering)
>   4. Re: [PATCH] xen/blkback: Squash the discard support for
>      'file' and 'phy' type. (Konrad Rzeszutek Wilk)
>   5. Re: libxl and unsigned types (Jan Beulich)
>
>
>----------------------------------------------------------------------
>
>Message: 1
>Date: Wed, 14 Mar 2012 11:48:10 -0400
>From: Zhigang Wang <zhigang.x.wang@xxxxxxxxxx>
>To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>,
>       Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
>Subject: Re: [Xen-devel] xl: expose max_cpu_id from `xl info`
>Message-ID: <4F60BDBA.3040209@xxxxxxxxxx>
>Content-Type: text/plain; charset="iso-8859-1"
>
>On 03/14/2012 06:45 AM, Andrew Cooper wrote:
>> On 13/03/12 19:15, Zhigang Wang wrote:
>>> On 03/13/2012 12:52 PM, Andrew Cooper wrote:
>>>> This will allow userspace to reason with the total number of CPUs, not
>>>> just the current number of online CPUs.
>>>>
>>>> -- Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer T: +44 (0)1223 
>>>> 225
>>>> 900, http://www.citrix.com
>>>>
>>>> xl-info-max_cpu_id.patch
>>>>
>>>>
>>>> # HG changeset patch
>>>> # Parent 5d20d2f6ffed0a49f030f04a8870f1926babbcbf
>>>> xl: display max_cpu_ids for xl info
>>>>
>>>> Expose "max_cpu_id" in stdout from `xl info`
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>>
>>>> diff -r 5d20d2f6ffed tools/libxl/xl_cmdimpl.c
>>>> --- a/tools/libxl/xl_cmdimpl.c
>>>> +++ b/tools/libxl/xl_cmdimpl.c
>>>> @@ -3734,6 +3734,7 @@ static void output_physinfo(void)
>>>>      }
>>>>  
>>>>      printf("nr_cpus                : %d\n", info.nr_cpus);
>>>> +    printf("max_cpu_id             : %d\n", info.max_cpu_id);
>>>
>>> Is this true: nr_cpus = max_cpu_id + 1 ?
>>>
>>> Zhigang
>>
>> if and only if all cpus are online.
>>
>> nr_cpus is set to nr_online_cpus() in the hypercall, while max_cpu_id is set
>> to nr_cpu_ids-1.
>>
>> nr_cpus is liable to change during runtime, while max_cpu_id is not.
>>
>> ~Andrew
>
>If this is the case, I think nr_cpus is a bit confusing. Can we make it always
>representing the number of physical cpus (from hypervisor's viewpoint, not 
>dom0)?
>
>Zhigang
>
>>
>>>
>>>>      printf("nr_nodes               : %d\n", info.nr_nodes);
>>>>      printf("cores_per_socket       : %d\n", info.cores_per_socket);
>>>>      printf("threads_per_core       : %d\n", info.threads_per_core);
>>>>
>>>>
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@xxxxxxxxxxxxx
>>>> http://lists.xen.org/xen-devel
>>>
>>
>> -- 
>> Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
>> T: +44 (0)1223 225 900, http://www.citrix.com
>
>-------------- next part --------------
>An HTML attachment was scrubbed...
>URL: 
><http://lists.xen.org/archives/html/xen-devel/attachments/20120314/ca02e86a/attachment.html>
>
>------------------------------
>
>Message: 2
>Date: Wed, 14 Mar 2012 16:02:24 +0000
>From: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
>To: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
>Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>, Jan Beulich
>       <JBeulich@xxxxxxxx>,    xen-devel <xen-devel@xxxxxxxxxxxxx>
>Subject: Re: [Xen-devel] libxl and unsigned types
>Message-ID:
>       <CAFLBxZarXcgXbyAu4gjQpdpsBkH7gS37mg0hhWJ0wgBn18288g@xxxxxxxxxxxxxx>
>Content-Type: text/plain; charset=ISO-8859-1
>
>On Wed, Mar 14, 2012 at 11:22 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 
>wrote:
>> Jan Beulich writes ("Re: [Xen-devel] [PATCH] libxl: don't accept negative 
>> disk or partition indexes"):
>>> I would call this surprising only if you think purely mathematically
>>> (which you shouldn't when programming in any language that uses
>>> finite width data types).
>>
>> I think this is a coding style question. ?It isn't addressed in
>> CODING_STYLE and the current code is (as ever) inconsistent, but we
>> should make a specific decision and stick to it for new changes at
>> least.
>>
>>> Instead I find it rather natural to actively use those properties
>>> that you call surprising, and in particular to use unsigned types
>>> for variables that can't (or shouldn't) have negative values
>>
>> The problem is that the cannot-be-negative property doesn't just apply
>> to the type (eg, count) in question. ?It also propagates to values
>> derived from it by arithmetic. ?Attempts, for example, to calculate
>> remaining space, or differences of any kind, go badly wrong.
>>
>> I think the correct approach is to use signed types and, at
>> appropriately points, explicitly limit incoming values to small enough
>> subsets of their types that arithmetic overflow cannot occur anywhere.
>>
>>> (not the least because this tends to produce better
>>> code, as no sign-extension is necessary when using such variables
>>> e.g. as array indexes).
>>
>> This is not a consideration in libxl because libxl doesn't contain hot
>> paths.
>>
>>> ? Plus I'd be curious where you would see fit
>>> for unsigned types (or whether you consider them evil altogether).
>>
>> They are useful for bitfields, values whose abstract types are
>> circular orders, and the like. ?Not things which are supposed to model
>> a subset of the mathematical integers.
>
>FWIW, overall ijc's proposal seems sensible to me.
>
> -George
>
>
>
>------------------------------
>
>Message: 3
>Date: Wed, 14 Mar 2012 17:03:03 +0100
>From: Olaf Hering <olaf@xxxxxxxxx>
>To: xen-devel@xxxxxxxxxxxxxxxxxxx
>Subject: [Xen-devel] [PATCH] tools/blktap: reorder MEMSHR_DIR to fix
>       CFLAGS
>Message-ID: <43d9bd95f981259776cb.1331740983@xxxxxxxxxxxx>
>Content-Type: text/plain; charset="us-ascii"
>
># HG changeset patch
># User Olaf Hering <olaf@xxxxxxxxx>
># Date 1331740943 -3600
># Node ID 43d9bd95f981259776cb6e3ea7fd7c536fa89135
># Parent  773d0367087212c43faf8cdcc21cf443b1ea0046
>tools/blktap: reorder MEMSHR_DIR to fix CFLAGS
>
>In blktap2 MEMSHR_DIR is used before it is set. This removes the
>required -D_GNU_SOURCE from CFLAGS, its used as option for -I
>Fix this by moving memshr related flags to the place where its actually
>used.
>The failure is a missing O_DIRECT define.
>
>Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
>
>diff -r 773d03670872 -r 43d9bd95f981 tools/blktap/drivers/Makefile
>--- a/tools/blktap/drivers/Makefile
>+++ b/tools/blktap/drivers/Makefile
>@@ -3,14 +3,12 @@ include $(XEN_ROOT)/tools/Rules.mk
> 
> IBIN         = blktapctrl tapdisk
> QCOW_UTIL    = img2qcow qcow2raw qcow-create
>-MEMSHR_DIR   = ../../memshr
> 
> CFLAGS   += -Werror
> CFLAGS   += -Wno-unused
> CFLAGS   += -I../lib
> CFLAGS   += $(CFLAGS_libxenctrl)
> CFLAGS   += $(CFLAGS_libxenstore)
>-CFLAGS   += -I $(MEMSHR_DIR)
> CFLAGS   += -D_GNU_SOURCE
> 
> ifeq ($CONFIG_GCRYPT,y)
>@@ -23,7 +21,9 @@ endif
> 
> MEMSHRLIBS :=
> ifeq ($(CONFIG_Linux), y)
>+MEMSHR_DIR   = ../../memshr
> CFLAGS += -DMEMSHR
>+CFLAGS += -I $(MEMSHR_DIR)
> MEMSHRLIBS += $(MEMSHR_DIR)/libmemshr.a
> endif
> 
>diff -r 773d03670872 -r 43d9bd95f981 tools/blktap2/drivers/Makefile
>--- a/tools/blktap2/drivers/Makefile
>+++ b/tools/blktap2/drivers/Makefile
>@@ -14,7 +14,6 @@ CFLAGS    += -Wno-unused
> CFLAGS    += -fno-strict-aliasing
> CFLAGS    += -I$(BLKTAP_ROOT)/include -I$(BLKTAP_ROOT)/drivers
> CFLAGS    += $(CFLAGS_libxenctrl)
>-CFLAGS    += -I $(MEMSHR_DIR)
> CFLAGS    += -D_GNU_SOURCE
> CFLAGS    += -DUSE_NFS_LOCKS
> 
>@@ -38,11 +37,11 @@ else
> tapdisk2 tapdisk-stream tapdisk-diff $(QCOW_UTIL): AIOLIBS := -laio
> endif
> 
>-MEMSHR_DIR = $(XEN_ROOT)/tools/memshr
>-
> MEMSHRLIBS :=
> ifeq ($(CONFIG_Linux), __fixme__)
>+MEMSHR_DIR = $(XEN_ROOT)/tools/memshr
> CFLAGS += -DMEMSHR
>+CFLAGS += -I $(MEMSHR_DIR)
> MEMSHRLIBS += -L$(XEN_ROOT)/tools/libxc -lxenctrl $(MEMSHR_DIR)/libmemshr.a
> endif
> 
>
>
>
>------------------------------
>
>Message: 4
>Date: Wed, 14 Mar 2012 12:03:27 -0400
>From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>To: Jan Beulich <JBeulich@xxxxxxxx>
>Cc: axboe@xxxxxxxxx, Li Dongyang <lidongyang@xxxxxxxx>,
>       xen-devel@xxxxxxxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx
>Subject: Re: [Xen-devel] [PATCH] xen/blkback: Squash the discard
>       support for 'file' and 'phy' type.
>Message-ID: <20120314160327.GC16960@xxxxxxxxxxxxxxxxxxx>
>Content-Type: text/plain; charset=us-ascii
>
>On Wed, Mar 14, 2012 at 09:48:21AM +0000, Jan Beulich wrote:
>> >>> On 13.03.12 at 23:52, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
>> >>> wrote:
>> > The only reason for the distinction was for the special case of
>> > 'file' (which is assumed to be loopback device), was to reach inside
>> > the loopback device, find the underlaying file, and call fallocate on it.
>> > Fortunately "xen-blkback: convert hole punching to discard request on
>> > loop devices" removes that use-case and we now based the discard
>> > support based on blk_queue_discard(q) and extract all appropriate
>> > parameters from the 'struct request_queue'.
>> > 
>> > CC: Li Dongyang <lidongyang@xxxxxxxxxx>
>> > CC: Jan Beulich <JBeulich@xxxxxxxx>
>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>> 
>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>> 
>> (with a few minor remarks below)
>> 
>> > --- a/drivers/block/xen-blkback/blkback.c
>> > +++ b/drivers/block/xen-blkback/blkback.c
>> > @@ -419,21 +419,17 @@ static int dispatch_discard_io(struct xen_blkif 
>> > *blkif,
>> >    int err = 0;
>> >    int status = BLKIF_RSP_OKAY;
>> >    struct block_device *bdev = blkif->vbd.bdev;
>> > -
>> > +  unsigned long secure = 0;
>> 
>> Mind keeping the blank line and dropping the pointless initializer (which
>> future gcc is likely going to be warning about)?
>
><nods>
>> 
>> >    blkif->st_ds_req++;
>> >  
>> >    xen_blkif_get(blkif);
>> > -  if (blkif->blk_backend_type == BLKIF_BACKEND_PHY ||
>> > -      blkif->blk_backend_type == BLKIF_BACKEND_FILE) {
>> > -          unsigned long secure = (blkif->vbd.discard_secure &&
>> > -                  (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
>> > -                  BLKDEV_DISCARD_SECURE : 0;
>> > -          err = blkdev_issue_discard(bdev,
>> > -                          req->u.discard.sector_number,
>> > -                          req->u.discard.nr_sectors,
>> > -                          GFP_KERNEL, secure);
>> > -  } else
>> > -          err = -EOPNOTSUPP;
>> > +  secure = (blkif->vbd.discard_secure &&
>> > +           (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
>> > +           BLKDEV_DISCARD_SECURE : 0;
>> > +
>> > +  err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
>> > +                             req->u.discard.nr_sectors,
>> > +                             GFP_KERNEL, secure);
>> >  
>> >    if (err == -EOPNOTSUPP) {
>> >            pr_debug(DRV_PFX "discard op failed, not supported\n");
>> > --- a/drivers/block/xen-blkback/xenbus.c
>> > +++ b/drivers/block/xen-blkback/xenbus.c
>> > @@ -393,52 +393,37 @@ int xen_blkbk_discard(struct xenbus_transaction xbt, 
>> > struct backend_info *be)
>> >    char *type;
>> >    int err;
>> >    int state = 0;
>> > +  struct block_device *bdev = be->blkif->vbd.bdev;
>> > +  struct request_queue *q = bdev_get_queue(bdev);
>> >  
>> > -  type = xenbus_read(XBT_NIL, dev->nodename, "type", NULL);
>> > -  if (!IS_ERR(type)) {
>> > -          if (strncmp(type, "file", 4) == 0) {
>> > -                  state = 1;
>> > -                  blkif->blk_backend_type = BLKIF_BACKEND_FILE;
>> > +  if (blk_queue_discard(q)) {
>> > +          err = xenbus_printf(xbt, dev->nodename,
>> > +                  "discard-granularity", "%u",
>> > +                  q->limits.discard_granularity);
>> > +          if (err) {
>> > +                  xenbus_dev_fatal(dev, err,
>> > +                          "writing discard-granularity");
>> > +                  goto kfree;
>> 
>> Unrelated to the patch here, but failure to write any sort of extension
>> data shouldn't be considered fatal - the backend can well work without
>> these, and it should be left to the frontend to decide whether it wants
>> to live without the unavailable extensions.
>
><nods>. Let me whip up another patch that removes these 'fatal' cases
>and is based on this one.
>
>> 
>> Jan
>> 
>> > +          }
>> > +          err = xenbus_printf(xbt, dev->nodename,
>> > +                  "discard-alignment", "%u",
>> > +                  q->limits.discard_alignment);
>> > +          if (err) {
>> > +                  xenbus_dev_fatal(dev, err,
>> > +                          "writing discard-alignment");
>> > +                  goto kfree;
>> >            }
>> > -          if (strncmp(type, "phy", 3) == 0) {
>> > -                  struct block_device *bdev = be->blkif->vbd.bdev;
>> > -                  struct request_queue *q = bdev_get_queue(bdev);
>> > -                  if (blk_queue_discard(q)) {
>> > -                          err = xenbus_printf(xbt, dev->nodename,
>> > -                                  "discard-granularity", "%u",
>> > -                                  q->limits.discard_granularity);
>> > -                          if (err) {
>> > -                                  xenbus_dev_fatal(dev, err,
>> > -                                          "writing discard-granularity");
>> > -                                  goto kfree;
>> > -                          }
>> > -                          err = xenbus_printf(xbt, dev->nodename,
>> > -                                  "discard-alignment", "%u",
>> > -                                  q->limits.discard_alignment);
>> > -                          if (err) {
>> > -                                  xenbus_dev_fatal(dev, err,
>> > -                                          "writing discard-alignment");
>> > -                                  goto kfree;
>> > -                          }
>> > -                          state = 1;
>> > -                          blkif->blk_backend_type = BLKIF_BACKEND_PHY;
>> > -                  }
>> > -                  /* Optional. */
>> > -                  err = xenbus_printf(xbt, dev->nodename,
>> > -                          "discard-secure", "%d",
>> > -                          blkif->vbd.discard_secure);
>> > -                  if (err) {
>> > -                          xenbus_dev_fatal(dev, err,
>> > +          state = 1;
>> > +          /* Optional. */
>> > +          err = xenbus_printf(xbt, dev->nodename,
>> > +                              "discard-secure", "%d",
>> > +                              blkif->vbd.discard_secure);
>> > +          if (err) {
>> > +                  xenbus_dev_fatal(dev, err,
>> >                                    "writting discard-secure");
>> > -                          goto kfree;
>> > -                  }
>> > +                  goto kfree;
>> >            }
>> > -  } else {
>> > -          err = PTR_ERR(type);
>> > -          xenbus_dev_fatal(dev, err, "reading type");
>> > -          goto out;
>> >    }
>> > -
>> >    err = xenbus_printf(xbt, dev->nodename, "feature-discard",
>> >                        "%d", state);
>> >    if (err)
>> 
>
>
>
>------------------------------
>
>Message: 5
>Date: Wed, 14 Mar 2012 16:14:10 +0000
>From: "Jan Beulich" <JBeulich@xxxxxxxx>
>To: "George Dunlap" <George.Dunlap@xxxxxxxxxxxxx>,     "Ian Jackson"
>       <Ian.Jackson@xxxxxxxxxxxxx>
>Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxx>, xen-devel
>       <xen-devel@xxxxxxxxxxxxx>
>Subject: Re: [Xen-devel] libxl and unsigned types
>Message-ID: <4F60D1E2020000780007868C@xxxxxxxxxxxxxxxxxxxx>
>Content-Type: text/plain; charset=US-ASCII
>
>>>> On 14.03.12 at 17:02, George Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote:
>> On Wed, Mar 14, 2012 at 11:22 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> 
>> wrote:
>>> Jan Beulich writes ("Re: [Xen-devel] [PATCH] libxl: don't accept negative 
>> disk or partition indexes"):
>>>> I would call this surprising only if you think purely mathematically
>>>> (which you shouldn't when programming in any language that uses
>>>> finite width data types).
>>>
>>> I think this is a coding style question.  It isn't addressed in
>>> CODING_STYLE and the current code is (as ever) inconsistent, but we
>>> should make a specific decision and stick to it for new changes at
>>> least.
>>>
>>>> Instead I find it rather natural to actively use those properties
>>>> that you call surprising, and in particular to use unsigned types
>>>> for variables that can't (or shouldn't) have negative values
>>>
>>> The problem is that the cannot-be-negative property doesn't just apply
>>> to the type (eg, count) in question.  It also propagates to values
>>> derived from it by arithmetic.  Attempts, for example, to calculate
>>> remaining space, or differences of any kind, go badly wrong.
>>>
>>> I think the correct approach is to use signed types and, at
>>> appropriately points, explicitly limit incoming values to small enough
>>> subsets of their types that arithmetic overflow cannot occur anywhere.
>>>
>>>> (not the least because this tends to produce better
>>>> code, as no sign-extension is necessary when using such variables
>>>> e.g. as array indexes).
>>>
>>> This is not a consideration in libxl because libxl doesn't contain hot
>>> paths.
>>>
>>>>   Plus I'd be curious where you would see fit
>>>> for unsigned types (or whether you consider them evil altogether).
>>>
>>> They are useful for bitfields, values whose abstract types are
>>> circular orders, and the like.  Not things which are supposed to model
>>> a subset of the mathematical integers.
>> 
>> FWIW, overall ijc's proposal seems sensible to me.
>
>I hadn't seen one, and the mail archive doesn't show one either.
>Or did you mean IanJ's reply above (in which case I continue to be
>of a different opinion, and hope I won't be forbidden to use unsigned
>types namely in hypervisor code - if on the tools side maintainers
>decide to do so I guess I'll have to try to cope with that)? Or is ijc not
>equivalent to IanC?
>
>Jan
>
>
>
>
>------------------------------
>
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@xxxxxxxxxxxxx
>http://lists.xen.org/xen-devel
>
>
>End of Xen-devel Digest, Vol 85, Issue 212
>******************************************
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel