Re: [libvirt] [PATCHv2] nodedev: add RDMA and tx-udp_tnl-segmentation NIC capabilities

[Moshe Levi]

> -Original Message-
> From: John Ferlan [mailto:jfer...@redhat.com]
> Sent: Tuesday, July 21, 2015 1:06 AM
> To: Moshe Levi; libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCHv2] nodedev: add RDMA and tx-udp_tnl-
> segmentation NIC capabilities
> 
> 
> 
> On 07/19/2015 06:11 AM, Moshe Levi wrote:
> > Adding functionality to libvirt that will allow it query the interface
> > for the availability of RDMA and tx-udp_tnl-segmentation Offloading
> > NIC capabilities
> >
> > Here is an example of the feature XML definition:
> >
> > 
> > net_eth4_90_e2_ba_5e_a5_45
> >
> /sys/devices/pci:00/:00:03.0/:08:00.1/net/eth4
> >   pci__08_00_1
> >   
> > eth4
> > 90:e2:ba:5e:a5:45
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >   
> > 
> > ---
> >  configure.ac  |2 +-
> >  docs/formatnode.html.in   |2 +
> >  src/conf/device_conf.c|4 +-
> >  src/conf/device_conf.h|2 +
> >  src/util/virnetdev.c  |  141 
> > +++--
> >  src/util/virnetdev.h  |1 +
> >  tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml |2 +
> >  tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml |2 +
> >  8 files changed, 144 insertions(+), 12 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac index a7f38e8..70b3ef3 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -390,7 +390,7 @@ AC_CHECK_TYPE([struct ifreq],
> >]])
> >
> >  AC_CHECK_DECLS([ETH_FLAG_TXVLAN, ETH_FLAG_NTUPLE,
> ETH_FLAG_RXHASH, ETH_FLAG_LRO,
> > -ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS],
> > +ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS,
> > + ETHTOOL_GFEATURES],
> >[], [], [[#include 
> >]])
> >
> > diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in index
> > 3ff1bef..ed00af5 100644
> > --- a/docs/formatnode.html.in
> > +++ b/docs/formatnode.html.in
> > @@ -199,6 +199,8 @@
> >  txvlantx-vlan-offload
> >  ntuplentuple-filters
> >
> > rxhashreceive-hashing
> > +rdmaremote-direct-memory-
> access
> > +
> > + txudptnltx-udp-tunnel-
> segmentation
> >  
> >
> >capability diff --git
> > a/src/conf/device_conf.c b/src/conf/device_conf.c index
> > 98808e2..e7b7957 100644
> > --- a/src/conf/device_conf.c
> > +++ b/src/conf/device_conf.c
> > @@ -51,7 +51,9 @@ VIR_ENUM_IMPL(virNetDevFeature,
> >"rxvlan",
> >"txvlan",
> >"ntuple",
> > -  "rxhash")
> > +  "rxhash",
> > +  "rdma",
> > +  "txudptnl")
> >
> >  int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr)  { diff
> > --git a/src/conf/device_conf.h b/src/conf/device_conf.h index
> > 7ea90f6..40a2b3d 100644
> > --- a/src/conf/device_conf.h
> > +++ b/src/conf/device_conf.h
> > @@ -74,6 +74,8 @@ typedef enum {
> >  VIR_NET_DEV_FEAT_TXVLAN,
> >  VIR_NET_DEV_FEAT_NTUPLE,
> >  VIR_NET_DEV_FEAT_RXHASH,
> > +VIR_NET_DEV_FEAT_RDMA,
> > +VIR_NET_DEV_FEAT_TXUDPTNL,
> >  VIR_NET_DEV_FEAT_LAST
> >  } virNetDevFeature;
> >
> > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index
> > e4fcd81..0dcb42d 100644
> > --- a/src/util/virnetdev.c
> > +++ b/src/util/virnetdev.c
> > @@ -87,6 +87,14 @@ VIR_LOG_INIT("util.netdev");  # define
> > VIR_IFF_ALLMULTI 0  #endif
> >
> > +#define RESOURCE_FILE_LEN 4096
> > +#define TX_UDP_TNL 25
> > +#define GFEATURES_SIZE 2
> > +#define FEATURE_WORD(blocks, index, field)  ((blocks)[(index) /
> 32U].field)
> > +#define FEATURE_FIELD_FLAG(index)  (1U << (index) % 32U)
> > +#define FEATURE_BIT_IS_SET(blocks, index, field)\
> > +(FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
> > +
> >  typedef enum {
> >  VIR_MCAST_TYPE_INDEX_TOKEN,
> >  VIR_MCAST_TYPE_NAME_TOKEN,
> > @@ -1868,7 +1876,6 @@ virNetDevReplaceMacAddress(const char
> *linkdev,
> >  goto cleanup;
> >
> >  ret = 0;
> > -
> >   cleanup:
> >  VIR_FREE(path);
> >  return ret;
> > @@ -2858,9 +2865,9 @@ static int virNetDevGetMulticastTable(const char
> *ifname,
> >  }
> >
> >  ret = 0;
> > +
> >   cleanup:
> >  virNetDevMcastListClear(&mcast);
> > -
> >  return ret;
> >  }
> >
> > @@ -2943,11 +2950,76 @@ int virNetDevGetRxFilter(const char *ifname,
> >  return ret;
> >  }
> >
> > +
> > +/**
> > + * virNetDevRDMAFeature
> > + * This function checks for the availability of RDMA feature
> > + * and add it to bitmap
> > + *
> > + * @ifname: name of the interface
> > + * @out: add RDMA feature if exist to bitmap
> > + *
> > + * Returns 0 on success, -1 on failure.
> > + */
> > +static int
> > +virNetDevRDMAFeature(const char *ifname,
> > + virBitmapPtr 

Re: [libvirt] [PATCH 03/13] virsh: blockjob: Split out vshBlockJobSetSpeed from blockJobImpl

[Peter Krempa]

On Mon, Jul 20, 2015 at 16:47:38 -0400, John Ferlan wrote:
> 
> 
> On 07/15/2015 12:33 PM, Peter Krempa wrote:
> > ---
> >  tools/virsh-domain.c | 33 -
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> > index 24f53ea..e3f7220 100644
> > --- a/tools/virsh-domain.c
> > +++ b/tools/virsh-domain.c
> > @@ -1666,7 +1666,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
> > 
> >  typedef enum {
> >  VSH_CMD_BLOCK_JOB_ABORT,
> > -VSH_CMD_BLOCK_JOB_SPEED,
> >  VSH_CMD_BLOCK_JOB_PULL,
> >  VSH_CMD_BLOCK_JOB_COMMIT,
> >  } vshCmdBlockJobMode;
> > @@ -1701,10 +1700,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
> >  if (virDomainBlockJobAbort(dom, path, flags) < 0)
> >  goto cleanup;
> >  break;
> > -case VSH_CMD_BLOCK_JOB_SPEED:
> > -if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0)
> > -goto cleanup;
> > -break;
> >  case VSH_CMD_BLOCK_JOB_PULL:
> >  if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0)
> >  goto cleanup;
> > @@ -2537,6 +2532,26 @@ vshBlockJobInfo(vshControl *ctl,
> > 
> > 
> >  static bool
> > +vshBlockJobSetSpeed(vshControl *ctl,
> > +const vshCmd *cmd,
> > +virDomainPtr dom,
> > +const char *path)
> > +{
> > +unsigned long bandwidth;
> > +
> > +if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) {
> > +vshError(ctl, "%s", _("bandwidth must be a number"));
> 
> vshCommandOptULInternal already emits an error message when ret < 0:
> 
> "Numeric value '%s' for <%s> option is malformed or out of range"
> 
> This is repeated in patch 5/13 and 6/13
> 
> Did you intend to have two messages?

Actually no I didn't. This patch is pretty old and pre-dates the
refactor where vshCommandOptULWrap started reporting it's own error.
I've just added the new argument in conflict resolution, but didn't
bother checking why it was added. Thanks for pointing out.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCHv2 02/17] conf: pay attention to bus minSlot/maxSlot when autoassigning PCI addresses

[Laine Stump]

BTW, in case anyone is hesitant to ACK this patch due to concern of
regressions - aside from my working through it by hand, there are
multiple tests (including for example "pci-bridge-many-disks") which
will fail if the exact same bus/slot is not assigned for many devices,
so I am certain that the behavior wrt pci-root and pci-bridge is exactly
the same as before the patch. (likewise, subsequent patches that
introduce new controller types have tests that rely on the
auto-assignment working properly for multiple controllers that have no
explicit PCI address).

On 07/17/2015 02:43 PM, Laine Stump wrote:
> The function that auto-assigns PCI addresses was written with the
> hardcoded assumptions that any PCI bus would have slots available
> starting at 1 and ending at 31. This isn't true for many types of
> controller (some have a single slot/port at 0, some have slots/ports
> from 0 to 31). This patch updates that function to remove the
> hardcoded assumptions. It will properly find/assign addresses for
> devices that can only connect to pcie-(root|downstream)-port (which
> have minSlot/maxSlot of 0/0) or a pcie-switch-upstream-port (0/31).
>
> It still will not auto-create a new bus of the proper kind for these
> connections when one doesn't exist, that task is for another day.
> ---
> new in V2
>
> src/conf/domain_addr.c | 65 +-
>  1 file changed, 38 insertions(+), 27 deletions(-)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 2be98c5..bc09279 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -471,24 +471,30 @@ 
> virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
> virDomainPCIConnectFlags flags)
>  {
>  /* default to starting the search for a free slot from
> - * :00:00.0
> + * the first slot of domain 0 bus 0...
>   */
>  virDevicePCIAddress a = { 0, 0, 0, 0, false };
>  char *addrStr = NULL;
>  
> -/* except if this search is for the exact same type of device as
> - * last time, continue the search from the previous match
> - */
> -if (flags == addrs->lastFlags)
> -a = addrs->lastaddr;
> -
>  if (addrs->nbuses == 0) {
>  virReportError(VIR_ERR_XML_ERROR, "%s", _("No PCI buses available"));
>  goto error;
>  }
>  
> -/* Start the search at the last used bus and slot */
> -for (a.slot++; a.bus < addrs->nbuses; a.bus++) {
> +/* ...unless this search is for the exact same type of device as
> + * last time, then continue the search from the next slot after
> + * the previous match.
> + */
> +if (flags == addrs->lastFlags) {
> +a = addrs->lastaddr;
> +if (++a.slot > addrs->buses[a.bus].maxSlot &&
> +++a.bus < addrs->nbuses)
> +a.slot = addrs->buses[a.bus].minSlot;
> +} else {
> +a.slot = addrs->buses[0].minSlot;
> +}
> +
> +while (a.bus < addrs->nbuses) {
>  if (!(addrStr = virDomainPCIAddressAsString(&a)))
>  goto error;
>  if (!virDomainPCIAddressFlagsCompatible(&a, addrStr,
> @@ -497,29 +503,33 @@ 
> virDomainPCIAddressGetNextSlot(virDomainPCIAddressSetPtr addrs,
>  VIR_FREE(addrStr);
>  VIR_DEBUG("PCI bus %.4x:%.2x is not compatible with the device",
>a.domain, a.bus);
> -continue;
> -}
> -for (; a.slot <= VIR_PCI_ADDRESS_SLOT_LAST; a.slot++) {
> -if (!virDomainPCIAddressSlotInUse(addrs, &a))
> -goto success;
> +} else {
> +while (a.slot <= addrs->buses[a.bus].maxSlot) {
> +if (!virDomainPCIAddressSlotInUse(addrs, &a))
> +goto success;
>  
> -VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> -  a.domain, a.bus, a.slot);
> +VIR_DEBUG("PCI slot %.4x:%.2x:%.2x already in use",
> +  a.domain, a.bus, a.slot);
> +a.slot++;
> +}
>  }
> -a.slot = 1;
> +if (++a.bus < addrs->nbuses)
> +a.slot = addrs->buses[a.bus].minSlot;
>  VIR_FREE(addrStr);
>  }
>  
>  /* There were no free slots after the last used one */
>  if (addrs->dryRun) {
> -/* a is already set to the first new bus and slot 1 */
> +/* a is already set to the first new bus */
>  if (virDomainPCIAddressSetGrow(addrs, &a, flags) < 0)
>  goto error;
> +/* this device will use the first slot of the new bus */
> +a.slot = addrs->buses[a.bus].minSlot;
>  goto success;
>  } else if (flags == addrs->lastFlags) {
>  /* Check the buses from 0 up to the last used one */
>  for (a.bus = 0; a.bus <= addrs->lastaddr.bus; a.bus++) {
> -addrStr = NULL;
> +a.slot = addrs->buses[a.bus].minSlot;
>  if (!(addrStr = vi

Re: [libvirt] [PATCHv2] nodedev: add RDMA and tx-udp_tnl-segmentation NIC capabilities

[John Ferlan]

On 07/19/2015 06:11 AM, Moshe Levi wrote:
> Adding functionality to libvirt that will allow
> it query the interface for the availability of RDMA and
> tx-udp_tnl-segmentation Offloading NIC capabilities
> 
> Here is an example of the feature XML definition:
> 
> 
> net_eth4_90_e2_ba_5e_a5_45
>   /sys/devices/pci:00/:00:03.0/:08:00.1/net/eth4
>   pci__08_00_1
>   
> eth4
> 90:e2:ba:5e:a5:45
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>   
> 
> ---
>  configure.ac  |2 +-
>  docs/formatnode.html.in   |2 +
>  src/conf/device_conf.c|4 +-
>  src/conf/device_conf.h|2 +
>  src/util/virnetdev.c  |  141 
> +++--
>  src/util/virnetdev.h  |1 +
>  tests/nodedevschemadata/net_00_13_02_b9_f9_d3.xml |2 +
>  tests/nodedevschemadata/net_00_15_58_2f_e9_55.xml |2 +
>  8 files changed, 144 insertions(+), 12 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index a7f38e8..70b3ef3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -390,7 +390,7 @@ AC_CHECK_TYPE([struct ifreq],
>]])
>  
>  AC_CHECK_DECLS([ETH_FLAG_TXVLAN, ETH_FLAG_NTUPLE, ETH_FLAG_RXHASH, 
> ETH_FLAG_LRO,
> -ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS],
> +ETHTOOL_GGSO, ETHTOOL_GGRO, ETHTOOL_GFLAGS, 
> ETHTOOL_GFEATURES],
>[], [], [[#include 
>]])
>  
> diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in
> index 3ff1bef..ed00af5 100644
> --- a/docs/formatnode.html.in
> +++ b/docs/formatnode.html.in
> @@ -199,6 +199,8 @@
>  txvlantx-vlan-offload
>  ntuplentuple-filters
>  rxhashreceive-hashing
> +
> rdmaremote-direct-memory-access
> +
> txudptnltx-udp-tunnel-segmentation
>  
>
>capability
> diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c
> index 98808e2..e7b7957 100644
> --- a/src/conf/device_conf.c
> +++ b/src/conf/device_conf.c
> @@ -51,7 +51,9 @@ VIR_ENUM_IMPL(virNetDevFeature,
>"rxvlan",
>"txvlan",
>"ntuple",
> -  "rxhash")
> +  "rxhash",
> +  "rdma",
> +  "txudptnl")
>  
>  int virDevicePCIAddressIsValid(virDevicePCIAddressPtr addr)
>  {
> diff --git a/src/conf/device_conf.h b/src/conf/device_conf.h
> index 7ea90f6..40a2b3d 100644
> --- a/src/conf/device_conf.h
> +++ b/src/conf/device_conf.h
> @@ -74,6 +74,8 @@ typedef enum {
>  VIR_NET_DEV_FEAT_TXVLAN,
>  VIR_NET_DEV_FEAT_NTUPLE,
>  VIR_NET_DEV_FEAT_RXHASH,
> +VIR_NET_DEV_FEAT_RDMA,
> +VIR_NET_DEV_FEAT_TXUDPTNL,
>  VIR_NET_DEV_FEAT_LAST
>  } virNetDevFeature;
>  
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index e4fcd81..0dcb42d 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -87,6 +87,14 @@ VIR_LOG_INIT("util.netdev");
>  # define VIR_IFF_ALLMULTI 0
>  #endif
>  
> +#define RESOURCE_FILE_LEN 4096
> +#define TX_UDP_TNL 25
> +#define GFEATURES_SIZE 2
> +#define FEATURE_WORD(blocks, index, field)  ((blocks)[(index) / 32U].field)
> +#define FEATURE_FIELD_FLAG(index)  (1U << (index) % 32U)
> +#define FEATURE_BIT_IS_SET(blocks, index, field)\
> +(FEATURE_WORD(blocks, index, field) & FEATURE_FIELD_FLAG(index))
> +
>  typedef enum {
>  VIR_MCAST_TYPE_INDEX_TOKEN,
>  VIR_MCAST_TYPE_NAME_TOKEN,
> @@ -1868,7 +1876,6 @@ virNetDevReplaceMacAddress(const char *linkdev,
>  goto cleanup;
>  
>  ret = 0;
> -
>   cleanup:
>  VIR_FREE(path);
>  return ret;
> @@ -2858,9 +2865,9 @@ static int virNetDevGetMulticastTable(const char 
> *ifname,
>  }
>  
>  ret = 0;
> +
>   cleanup:
>  virNetDevMcastListClear(&mcast);
> -
>  return ret;
>  }
>  
> @@ -2943,11 +2950,76 @@ int virNetDevGetRxFilter(const char *ifname,
>  return ret;
>  }
>  
> +
> +/**
> + * virNetDevRDMAFeature
> + * This function checks for the availability of RDMA feature
> + * and add it to bitmap
> + *
> + * @ifname: name of the interface
> + * @out: add RDMA feature if exist to bitmap
> + *
> + * Returns 0 on success, -1 on failure.
> + */
> +static int
> +virNetDevRDMAFeature(const char *ifname,
> + virBitmapPtr *out)
> +{
> +char *eth_devpath = NULL;
> +char *ib_devpath = NULL;
> +char *eth_res_buf = NULL;
> +char *ib_res_buf = NULL;
> +DIR *dirp = NULL;
> +struct dirent *dp;
> +int ret = -1;
> +
> +if (!(dirp = opendir(SYSFS_INFINIBAND_DIR))) {
> +virReportSystemError(errno,
> + _("Failed to opendir path '%s'"),
> + SYSFS_INFINIBAND_DIR);
> +goto cleanup;
> +}
> +
> +if (virAsprintf(ð_devpath, SYSFS_NE

Re: [libvirt] [PATCH 00/13] Improve virsh block job handling

[Eric Blake]

On 07/20/2015 02:48 PM, John Ferlan wrote:
> 
> 
> On 07/15/2015 12:33 PM, Peter Krempa wrote:
>> This series refactors the block job command code so that it is handled by
>> separate functions rather than one mega-method. Additionally this series then
>> fixes the routine for waiting for a block job.
>>
>> As it's perhaps obvious from the patches the API design for the block job 
>> APIs
>> is rather unfortunate for users.
>>
>> This series contains a few patches that already were submitted and partially
>> reviewed previously.
>>

> 
> Noted a couple of minor things, but looks good otherwise.

and I pointed out a few more

> 
> ACK series,

Concur. This makes virsh look a lot nicer, and the big benefit is that
it tries to address the data race bug where live block-commit --pivot
could read status too early and fail (probably because qemu was
reporting info.end of 0 before actually getting into the main loop of
the job).

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/13] virsh: Refactor block job waiting in cmdBlockPull

[Eric Blake]

On 07/15/2015 10:34 AM, Peter Krempa wrote:
> Introduce helper function that will provide logic for waiting for block
> job completion so the 3 open coded places can be unified and improved.
> 
> This patch introduces the whole logic and uses it to fix
> cmdBlockJobPull. The vshBlockJobWait funtion provides common logic for

s/funtion/function/

> block job waiting that should be robust enough to work across all
> previous versions of libvirt. Since virsh allows to pass user-provided

s/allows to pass/allows passing/

> strings as paths of block devices we can't reliably use block job events
> for detection of block job states so the function contains a great deal
> of fallback logic.
> ---
>  tools/virsh-domain.c | 326 
> ++-
>  1 file changed, 244 insertions(+), 82 deletions(-)
> 

> +/**
> + * vshBlockJobWait:
> + * @data: private data initialized by vshBlockJobWaitInit
> + *
> + * Waits for the block job to complete. This fucntion prefers to get an event

s/fucntion/function/


> +/* for two-phase jobs we will try to wait in the synchronized phase
> + * for event arival since 100% completion doesn't necessarily mean 
> that

s/arival/arrival/

> + * the block job has finished and can be terminated with success */
> +if (info.end == info.cur && --retries == 0) {

As I said in the other patch, if info.end == info.cur and they are
non-zero, that SHOULD be equivalent to the job being complete (a
blockpull finishing, or a two-phase job transitioning to synchronized);
where it gets tricky is if the two are equal but zero (as some jobs can
validly have info.end == 0, but most jobs just see info.end as 0 before
getting into the main loop of the job).  But it looks like you are doing
okay here.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/13] qemu: Update state of block job to READY only if it actually is ready

[Eric Blake]

On 07/15/2015 10:33 AM, Peter Krempa wrote:
> Few parts of the code looked at the current progress of and assumed that
> a two phase blockjob is in the _READY state as soon as the progress
> reached 100% (info.cur == info.end). In current versions of qemu this
> assumption is invalid and qemu exposes a new flag 'ready' in the
> query-block-jobs output that is set to true if the job is actually
> finished.
> 
> This patch adds internal data handling for reading the 'ready' flag and
> acting appropriately as long as the flag is present.
> 
> While this still doesn't fix the virsh client problem with two phase
> block jobs and the --pivot option, it at least improves the error
> message:
> 
> $ virsh blockcommit  --wait --verbose vm vda  --base vda[1] --active --pivot
> Block commit: [100 %]error: failed to pivot job for disk vda
> error: internal error: unable to execute QEMU command 'block-job-complete': 
> The active block job for device 'drive-virtio-disk0' cannot be completed
> 
> to
> 
> $ virsh blockcommit  --wait --verbose VM vda  --base vda[1] --active --pivot
> Block commit: [100 %]error: failed to pivot job for disk vda
> error: block copy still active: disk 'vda' not ready for pivot yet

Thanks for working on this.

I think part of the problem is that newer qemu races, and can report
info.cur == info.end == 0 very early in the process before starting to
report non-zero info.end, but info.ready is correctly false in that point.

You're right that this doesn't solve virsh racing when it sees info.cur
== info.end as a potential job complete condition (where special-casing
0 then runs into problems for no-op jobs where there really is no work
to do and the job can end instantly), but using the extra information
from qemu does look like an improvement.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/13] virsh: cmdBlockJob: Switch to declarative flag interlocking

[Eric Blake]

On 07/15/2015 10:33 AM, Peter Krempa wrote:
> Use the VSH_EXCLUSIVE_OPTIONS_VAR to interlock incompatible options
> ---
>  tools/virsh-domain.c | 33 +++--
>  1 file changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 7a18204..24f53ea 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -2542,26 +2542,31 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>  bool ret = false;
>  bool raw = vshCommandOptBool(cmd, "raw");
>  bool bytes = vshCommandOptBool(cmd, "bytes");
> -bool abortMode = (vshCommandOptBool(cmd, "abort") ||
> -  vshCommandOptBool(cmd, "async") ||
> -  vshCommandOptBool(cmd, "pivot"));
> -bool infoMode = vshCommandOptBool(cmd, "info") || raw;
> +bool abort = vshCommandOptBool(cmd, "abort");

Will that get you in trouble with older compilers that complain about
local variables shadowing global function names?

Other than that, ACK.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/13] virsh: blockjob: Extract block job info code into a separate function

[Eric Blake]

On 07/15/2015 10:33 AM, Peter Krempa wrote:
> cmdBlockJob will be converted to a hub that will call into the
> individual executor functions.
> ---
>  tools/virsh-domain.c | 91 
> +++-
>  1 file changed, 55 insertions(+), 36 deletions(-)

ACK.  It will be nice to finally get rid of this mis-directed multiplexing.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 11/13] virsh: Refactor block job waiting in cmdBlockPull

[John Ferlan]

On 07/15/2015 12:34 PM, Peter Krempa wrote:
> Introduce helper function that will provide logic for waiting for block
> job completion so the 3 open coded places can be unified and improved.
> 
> This patch introduces the whole logic and uses it to fix
> cmdBlockJobPull. The vshBlockJobWait funtion provides common logic for
> block job waiting that should be robust enough to work across all
> previous versions of libvirt. Since virsh allows to pass user-provided
> strings as paths of block devices we can't reliably use block job events
> for detection of block job states so the function contains a great deal
> of fallback logic.
> ---
>  tools/virsh-domain.c | 326 
> ++-
>  1 file changed, 244 insertions(+), 82 deletions(-)
> 

...

>  /*
>   * "blockcommit" command
>   */
> @@ -2660,19 +2881,11 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
>  bool verbose = vshCommandOptBool(cmd, "verbose");
>  bool async = vshCommandOptBool(cmd, "async");
>  int timeout = 0;
> -struct sigaction sig_action;
> -struct sigaction old_sig_action;
> -sigset_t sigmask, oldsigmask;
> -struct timeval start;
> -struct timeval curr;
>  const char *path = NULL;
>  const char *base = NULL;
>  unsigned long bandwidth = 0;
> -bool quit = false;
> -int abort_flags = 0;
> -int status = -1;
> -int cb_id = -1;
>  unsigned int flags = 0;
> +vshBlockJobWaitDataPtr bjWait = NULL;
> 
>  VSH_REQUIRE_OPTION("verbose", "wait");
>  VSH_REQUIRE_OPTION("async", "wait");
> @@ -2694,35 +2907,13 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
>  if (vshCommandOptBool(cmd, "keep-relative"))
>  flags |= VIR_DOMAIN_BLOCK_REBASE_RELATIVE;
> 
> -if (async)
> -abort_flags |= VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC;
> -
> -if (blocking) {
> -sigemptyset(&sigmask);
> -sigaddset(&sigmask, SIGINT);
> -
> -intCaught = 0;
> -sig_action.sa_sigaction = vshCatchInt;
> -sig_action.sa_flags = SA_SIGINFO;
> -sigemptyset(&sig_action.sa_mask);
> -sigaction(SIGINT, &sig_action, &old_sig_action);
> -
> -GETTIMEOFDAY(&start);
> -}
> -
>  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>  return false;
> 
> -virConnectDomainEventGenericCallback cb =
> -VIR_DOMAIN_EVENT_CALLBACK(vshBlockJobStatusHandler);
> -
> -if ((cb_id = virConnectDomainEventRegisterAny(ctl->conn,
> -  dom,
> -  
> VIR_DOMAIN_EVENT_ID_BLOCK_JOB,
> -  cb,
> -  &status,
> -  NULL)) < 0)
> -vshResetLibvirtError();
> +if (blocking &&
> +!(bjWait = vshBlockJobWaitInit(ctl, dom, path, _("Block Pull"), 
> verbose,
> +   timeout, async)))
> +goto cleanup;
> 
>  if (base || flags) {
>  if (virDomainBlockRebase(dom, path, base, bandwidth, flags) < 0)
> @@ -2738,61 +2929,32 @@ cmdBlockPull(vshControl *ctl, const vshCmd *cmd)
>  goto cleanup;
>  }
> 
> -while (blocking) {
> -virDomainBlockJobInfo info;
> -int result;
> -
> -pthread_sigmask(SIG_BLOCK, &sigmask, &oldsigmask);
> -result = virDomainGetBlockJobInfo(dom, path, &info, 0);
> -pthread_sigmask(SIG_SETMASK, &oldsigmask, NULL);
> +/* Execution continues here only if --wait or friends were specifed */

specified

This is also repeated in 12/13 and 13/13


> +switch (vshBlockJobWait(bjWait)) {
> +case -1:
> +goto cleanup;
> 
> -if (result < 0) {
> -vshError(ctl, _("failed to query job for disk %s"), path);
> +case VIR_DOMAIN_BLOCK_JOB_CANCELED:
> +vshPrint(ctl, "\n%s", _("Pull aborted"));
>  goto cleanup;
> -}
> -if (result == 0)
>  break;
> 
> -if (verbose)
> -vshPrintJobProgress(_("Block Pull"), info.end - info.cur, 
> info.end);
> -
> -GETTIMEOFDAY(&curr);
> -if (intCaught || (timeout &&
> -  (((int)(curr.tv_sec - start.tv_sec)  * 1000 +
> -(int)(curr.tv_usec - start.tv_usec) / 1000) >
> -   timeout))) {
> -vshDebug(ctl, VSH_ERR_DEBUG,
> - intCaught ? "interrupted" : "timeout");
> -intCaught = 0;
> -timeout = 0;
> -status = VIR_DOMAIN_BLOCK_JOB_CANCELED;
> -if (virDomainBlockJobAbort(dom, path, abort_flags) < 0) {
> -vshError(ctl, _("failed to abort job for disk %s"), path);
> -goto cleanup;
> -}
> -if (abort_flags & VIR_DOMAIN_BLOCK_JOB_ABORT_ASYNC)
> -break;
> -} else {
> 

Re: [libvirt] [PATCH 03/13] virsh: blockjob: Split out vshBlockJobSetSpeed from blockJobImpl

[John Ferlan]

On 07/15/2015 12:33 PM, Peter Krempa wrote:
> ---
>  tools/virsh-domain.c | 33 -
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 24f53ea..e3f7220 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -1666,7 +1666,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
> 
>  typedef enum {
>  VSH_CMD_BLOCK_JOB_ABORT,
> -VSH_CMD_BLOCK_JOB_SPEED,
>  VSH_CMD_BLOCK_JOB_PULL,
>  VSH_CMD_BLOCK_JOB_COMMIT,
>  } vshCmdBlockJobMode;
> @@ -1701,10 +1700,6 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
>  if (virDomainBlockJobAbort(dom, path, flags) < 0)
>  goto cleanup;
>  break;
> -case VSH_CMD_BLOCK_JOB_SPEED:
> -if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0)
> -goto cleanup;
> -break;
>  case VSH_CMD_BLOCK_JOB_PULL:
>  if (vshCommandOptStringReq(ctl, cmd, "base", &base) < 0)
>  goto cleanup;
> @@ -2537,6 +2532,26 @@ vshBlockJobInfo(vshControl *ctl,
> 
> 
>  static bool
> +vshBlockJobSetSpeed(vshControl *ctl,
> +const vshCmd *cmd,
> +virDomainPtr dom,
> +const char *path)
> +{
> +unsigned long bandwidth;
> +
> +if (vshCommandOptULWrap(ctl, cmd, "bandwidth", &bandwidth) < 0) {
> +vshError(ctl, "%s", _("bandwidth must be a number"));

vshCommandOptULInternal already emits an error message when ret < 0:

"Numeric value '%s' for <%s> option is malformed or out of range"

This is repeated in patch 5/13 and 6/13

Did you intend to have two messages?

John

> +return false;
> +}
> +
> +if (virDomainBlockJobSetSpeed(dom, path, bandwidth, 0) < 0)
> +return false;
> +
> +return true;
> +}
> +
> +
> +static bool
>  cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>  {
>  bool ret = false;
> @@ -2568,10 +2583,7 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
> 
>  if (abort || pivot || async)
>  return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_ABORT, NULL);
> -if (bandwidth)
> -return blockJobImpl(ctl, cmd, VSH_CMD_BLOCK_JOB_SPEED, NULL);
> 
> -/* Everything below here is for --info mode */
>  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>  goto cleanup;
> 
> @@ -2579,7 +2591,10 @@ cmdBlockJob(vshControl *ctl, const vshCmd *cmd)
>  if (vshCommandOptStringReq(ctl, cmd, "path", &path) < 0)
>  goto cleanup;
> 
> -ret = vshBlockJobInfo(ctl, dom, path, raw, bytes);
> +if (bandwidth)
> +ret = vshBlockJobSetSpeed(ctl, cmd, dom, path);
> +else
> +ret = vshBlockJobInfo(ctl, dom, path, raw, bytes);
> 
>   cleanup:
>  if (dom)
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/13] Improve virsh block job handling

[John Ferlan]

On 07/15/2015 12:33 PM, Peter Krempa wrote:
> This series refactors the block job command code so that it is handled by
> separate functions rather than one mega-method. Additionally this series then
> fixes the routine for waiting for a block job.
> 
> As it's perhaps obvious from the patches the API design for the block job APIs
> is rather unfortunate for users.
> 
> This series contains a few patches that already were submitted and partially
> reviewed previously.
> 
> Peter Krempa (13):
>   virsh: blockjob: Extract block job info code into a separate function
>   virsh: cmdBlockJob: Switch to declarative flag interlocking
>   virsh: blockjob: Split out vshBlockJobSetSpeed from blockJobImpl
>   virsh: block job: separate abort from blockJobImpl
>   virsh: Split out block pull implementation from blockJobImpl
>   virsh: Kill blockJobImpl by moving the final impl into cmdBlockCommit
>   virsh: Refactor argument checking in cmdBlockCommit
>   virsh: Refactor argument handling in cmdBlockCopy
>   virsh: Refactor argument handling in cmdBlockPull
>   qemu: Update state of block job to READY only if it actually is ready
>   virsh: Refactor block job waiting in cmdBlockPull
>   virsh: Refactor block job waiting in cmdBlockCommit
>   virsh: Refactor block job waiting in cmdBlockCopy
> 
>  src/qemu/qemu_driver.c   |  10 +-
>  src/qemu/qemu_monitor.h  |   1 +
>  src/qemu/qemu_monitor_json.c |   7 +
>  tools/virsh-domain.c | 962 
> ---
>  4 files changed, 551 insertions(+), 429 deletions(-)
> 

Noted a couple of minor things, but looks good otherwise.

ACK series,

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Christophe Fergeau]

On Mon, Jul 20, 2015 at 05:24:30PM +0100, Zeeshan Ali (Khattak) wrote:
> And all that data is completely irrelevant for the reason I mentioned
> again and again.

Now that we have the data, and that it goes your way, yes you can say
it's irrelevant ;) What if instead, it turned out only f22 was shipping a
new enough libvirt? I would have reverted your patch and added some
#ifdef.

> Please point to at least one actual example of any
> distro wanting to upgrade libvirt-glib to latest while wanting to keep
> an year old release of libvirt and you'll have a point.

The point of looking up that information was actually to be able to make
an informed decision as to whether there may be distros wanting to do
that or not. Which is why I was asking for that data, and I now feel
more confident that this is indeed not going to be an issue. It really
took 10 minutes to gather, much less time than arguing that this
information is not needed ;)

Christophe


pgpATcrc8UMxX.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v6 1/5] nodeinfo: Fix output on PPC64 KVM hosts

[Andrea Bolognani]

From: Shivaprasad G Bhat 

The nodeinfo is reporting incorrect number of cpus and incorrect host
topology on PPC64 KVM hosts. The KVM hypervisor on PPC64 needs only
the primary thread in a core to be online, and the secondaries offlined.
While scheduling a guest in, the kvm scheduler wakes up the secondaries to
run in guest context.

The host scheduling of the guests happen at the core level(as only primary
thread is online). The kvm scheduler exploits as many threads of the core
as needed by guest. Further, starting POWER8, the processor allows splitting
a physical core into multiple subcores with 2 or 4 threads each. Again, only
the primary thread in a subcore is online in the host. The KVM-PPC
scheduler allows guests to exploit all the offline threads in the subcore,
by bringing them online when needed.
(Kernel patches on split-core 
http://www.spinics.net/lists/kvm-ppc/msg09121.html)

Recently with dynamic micro-threading changes in ppc-kvm, makes sure
to utilize all the offline cpus across guests, and across guests with
different cpu topologies.
(https://www.mail-archive.com/kvm@vger.kernel.org/msg115978.html)

Since the offline cpus are brought online in the guest context, it is safe
to count them as online. Nodeinfo today discounts these offline cpus from
cpu count/topology calclulation, and the nodeinfo output is not of any help
and the host appears overcommited when it is actually not.

The patch carefully counts those offline threads whose primary threads are
online. The host topology displayed by the nodeinfo is also fixed when the
host is in valid kvm state.

Signed-off-by: Shivaprasad G Bhat 
Signed-off-by: Andrea Bolognani 
---
 src/libvirt_private.syms |   1 +
 src/nodeinfo.c   | 144 +--
 src/nodeinfo.h   |   1 +
 3 files changed, 142 insertions(+), 4 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7f42e40..46e535e 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1009,6 +1009,7 @@ nodeGetMemoryParameters;
 nodeGetMemoryStats;
 nodeGetOnlineCPUBitmap;
 nodeGetPresentCPUBitmap;
+nodeGetThreadsPerSubcore;
 nodeSetMemoryParameters;
 
 
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 31095a4..0fedc0e 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -31,6 +31,12 @@
 #include 
 #include 
 #include "conf/domain_conf.h"
+#include 
+#include 
+
+#if HAVE_LINUX_KVM_H
+# include 
+#endif
 
 #if defined(__FreeBSD__) || defined(__APPLE__)
 # include 
@@ -391,14 +397,15 @@ virNodeParseSocket(const char *dir,
  * filling arguments */
 static int
 ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
-ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
-ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(8)
-ATTRIBUTE_NONNULL(9)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(7)
+ATTRIBUTE_NONNULL(8) ATTRIBUTE_NONNULL(9)
+ATTRIBUTE_NONNULL(10)
 virNodeParseNode(const char *node,
  virArch arch,
  virBitmapPtr present_cpus_map,
  int npresent_cpus,
  virBitmapPtr online_cpus_map,
+ int threads_per_subcore,
  int *sockets,
  int *cores,
  int *threads,
@@ -491,7 +498,18 @@ virNodeParseNode(const char *node,
 continue;
 
 if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
-(*offline)++;
+if (threads_per_subcore > 0 &&
+cpu % threads_per_subcore != 0 &&
+virBitmapIsBitSet(online_cpus_map,
+  cpu - (cpu % threads_per_subcore))) {
+/* Secondary offline threads are counted as online when
+ * subcores are in use and the corresponding primary
+ * thread is online */
+processors++;
+} else {
+/* But they are counted as offline otherwise */
+(*offline)++;
+}
 continue;
 }
 
@@ -542,6 +560,12 @@ virNodeParseNode(const char *node,
 *cores = core;
 }
 
+if (threads_per_subcore > 0) {
+/* The thread count ignores offline threads, which means that only
+ * only primary threads have been considered so far. If subcores
+ * are in use, we need to also account for secondary threads */
+*threads *= threads_per_subcore;
+}
 ret = processors;
 
  cleanup:
@@ -560,6 +584,48 @@ virNodeParseNode(const char *node,
 return ret;
 }
 
+/* Check whether the host subcore configuration is valid.
+ *
+ * A valid configuration is one where no secondary thread is online;
+ * the primary thread in a subcore is always the first one */
+static bool
+nodeHasValidSubcoreConfiguration(const char *sysfs_prefix,
+ int threads_per_subcore)
+{
+virBitmapPtr online_cpus = NULL;
+int nonline_cpus;
+int cpu;
+bool ret = false;
+
+/* No point in checking if subcores are not in use */
+if (th

[libvirt] [PATCH v6 0/5] nodeinfo: Add support for subcores

[Andrea Bolognani]

Changes from v5 to v6:

  * updated to work on top of

  [PATCH v2 00/10] nodeinfo: Various cleanups

I'm only sending patch 1/5 to the list, because all the
other commits haven't changed since v5 and because
this way there's no need to perform moderation due to
the message size.

For the same reason, I've heavily edited the following
summary by cutting out everything that belonged to a
test case.

Cheers.


Andrea Bolognani (3):
  tests: Add subcores-default nodeinfo test
  tests: Add subcores-partial nodeinfo test
  tests: Add subcores-invalid nodeinfo test

Shivaprasad G Bhat (2):
  nodeinfo: Fix output on PPC64 KVM hosts
  tests: Prepare for subcore tests

 src/libvirt_private.syms   |   1 +
 src/nodeinfo.c | 144 -
 src/nodeinfo.h |   1 +
 tests/Makefile.am  |   6 +
 [...]
 tests/nodeinfomock.c   |  35 +
 tests/nodeinfotest.c   |   5 +-
 1348 files changed, 2122 insertions(+), 6 deletions(-)
 [...]
 create mode 100644 tests/nodeinfomock.c

-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 09/10] nodeinfo: Use a bitmap to keep track of node CPUs

[Andrea Bolognani]

Keep track of what CPUs belong to the current node while walking
through the sysfs node entry, so we don't need to do it a second
time immediately afterwards.

This also allows us to loop through all CPUs that are part of a
node in guaranteed ascending order, which is something that is
required for some upcoming changes.
---
 src/nodeinfo.c | 28 ++--
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index c97dece..2328a86 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -410,8 +410,10 @@ virNodeParseNode(const char *sysfs_prefix,
 struct dirent *cpudirent = NULL;
 virBitmapPtr present_cpumap = NULL;
 virBitmapPtr online_cpus_map = NULL;
+virBitmapPtr node_cpus_map = NULL;
 virBitmapPtr sockets_map = NULL;
 virBitmapPtr *cores_maps = NULL;
+int npresent_cpus;
 int sock_max = 0;
 int sock;
 int core;
@@ -432,10 +434,16 @@ virNodeParseNode(const char *sysfs_prefix,
 present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
 if (!present_cpumap)
 goto cleanup;
-online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
+online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
 if (!online_cpus_map)
 goto cleanup;
 
+npresent_cpus = virBitmapSize(present_cpumap);
+
+/* Keep track of the CPUs that belong to the current node */
+if (!(node_cpus_map = virBitmapNew(npresent_cpus)))
+goto cleanup;
+
 /* enumerate sockets in the node */
 if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
 goto cleanup;
@@ -447,6 +455,10 @@ virNodeParseNode(const char *sysfs_prefix,
 if (!virBitmapIsBitSet(present_cpumap, cpu))
 continue;
 
+/* Mark this CPU as part of the current node */
+if (virBitmapSetBit(node_cpus_map, cpu) < 0)
+goto cleanup;
+
 if (!virBitmapIsBitSet(online_cpus_map, cpu))
 continue;
 
@@ -480,13 +492,11 @@ virNodeParseNode(const char *sysfs_prefix,
 if (!(cores_maps[i] = virBitmapNew(ID_MAX + 1)))
 goto cleanup;
 
-/* iterate over all CPU's in the node */
-rewinddir(cpudir);
-while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
-if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
-continue;
+/* Iterate over all CPUs in the node, in ascending order */
+for (cpu = 0; cpu < npresent_cpus; cpu++) {
 
-if (!virBitmapIsBitSet(present_cpumap, cpu))
+/* Skip CPUs that are not part of the current node */
+if (!virBitmapIsBitSet(node_cpus_map, cpu))
 continue;
 
 if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
@@ -529,9 +539,6 @@ virNodeParseNode(const char *sysfs_prefix,
 *threads = siblings;
 }
 
-if (direrr < 0)
-goto cleanup;
-
 /* finalize the returned data */
 *sockets = virBitmapCountBits(sockets_map);
 
@@ -557,6 +564,7 @@ virNodeParseNode(const char *sysfs_prefix,
 virBitmapFree(cores_maps[i]);
 VIR_FREE(cores_maps);
 virBitmapFree(sockets_map);
+virBitmapFree(node_cpus_map);
 virBitmapFree(online_cpus_map);
 virBitmapFree(present_cpumap);
 
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 06/10] nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()

[Andrea Bolognani]

The new name makes it clear that the returned bitmap contains the
information about which CPUs are online, not eg. which CPUs are
present.

No behavioral change.
---
 src/libvirt_private.syms | 2 +-
 src/nodeinfo.c   | 6 +++---
 src/nodeinfo.h   | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b6347de..7f42e40 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -999,7 +999,6 @@ virLockManagerRelease;
 nodeAllocPages;
 nodeCapsInitNUMA;
 nodeGetCellsFreeMemory;
-nodeGetCPUBitmap;
 nodeGetCPUCount;
 nodeGetCPUMap;
 nodeGetCPUStats;
@@ -1008,6 +1007,7 @@ nodeGetInfo;
 nodeGetMemory;
 nodeGetMemoryParameters;
 nodeGetMemoryStats;
+nodeGetOnlineCPUBitmap;
 nodeGetPresentCPUBitmap;
 nodeSetMemoryParameters;
 
diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 69c15fc..3550e24 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1324,7 +1324,7 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix 
ATTRIBUTE_UNUSED)
 }
 
 virBitmapPtr
-nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
+nodeGetOnlineCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
 {
 #ifdef __linux__
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
@@ -1369,7 +1369,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix 
ATTRIBUTE_UNUSED)
 return cpumap;
 #else
 virReportError(VIR_ERR_NO_SUPPORT, "%s",
-   _("node cpumap not implemented on this platform"));
+   _("node online CPU map not implemented on this platform"));
 return NULL;
 #endif
 }
@@ -1691,7 +1691,7 @@ nodeGetCPUMap(const char *sysfs_prefix,
 if (!cpumap && !online)
 return nodeGetCPUCount(sysfs_prefix);
 
-if (!(cpus = nodeGetCPUBitmap(sysfs_prefix)))
+if (!(cpus = nodeGetOnlineCPUBitmap(sysfs_prefix)))
 goto cleanup;
 
 if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0)
diff --git a/src/nodeinfo.h b/src/nodeinfo.h
index 02af9c5..1810c1c 100644
--- a/src/nodeinfo.h
+++ b/src/nodeinfo.h
@@ -45,7 +45,7 @@ int nodeGetMemory(unsigned long long *mem,
   unsigned long long *freeMem);
 
 virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
-virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix);
+virBitmapPtr nodeGetOnlineCPUBitmap(const char *sysfs_prefix);
 int nodeGetCPUCount(const char *sysfs_prefix);
 
 int nodeGetMemoryParameters(virTypedParameterPtr params,
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 07/10] nodeinfo: Phase out cpu_set_t usage

[Andrea Bolognani]

Swap out all instances of cpu_set_t and replace them with virBitmap,
which some of the code was already using anyway.

The changes are pretty mechanical, with one notable exception: an
assumption has been added on the max value we can run into while
reading either socket_it or core_id.

While this specific assumption was not in place before, we were
using cpu_set_t improperly by not making sure not to set any bit
past CPU_SETSIZE or explicitly allocating bigger bitmaps; in fact
the default size of a cpu_set_t, 1024, is way too low to run our
testsuite, which includes core_id values in the 2000s.
---
 src/nodeinfo.c | 65 ++
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 3550e24..7d0e6b0 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -30,7 +30,6 @@
 #include 
 #include 
 #include 
-#include 
 #include "conf/domain_conf.h"
 
 #if defined(__FreeBSD__) || defined(__APPLE__)
@@ -388,19 +387,6 @@ virNodeParseSocket(const char *dir,
 return ret;
 }
 
-# ifndef CPU_COUNT
-static int
-CPU_COUNT(cpu_set_t *set)
-{
-size_t i, count = 0;
-
-for (i = 0; i < CPU_SETSIZE; i++)
-if (CPU_ISSET(i, set))
-count++;
-return count;
-}
-# endif /* !CPU_COUNT */
-
 /* parses a node entry, returning number of processors in the node and
  * filling arguments */
 static int
@@ -415,15 +401,18 @@ virNodeParseNode(const char *sysfs_prefix,
  int *threads,
  int *offline)
 {
+/* Biggest value we can expect to be used as either socket id
+ * or core id. Bitmaps will need to be sized accordingly */
+const int ID_MAX = 4095;
 int ret = -1;
 int processors = 0;
 DIR *cpudir = NULL;
 struct dirent *cpudirent = NULL;
 virBitmapPtr present_cpumap = NULL;
+virBitmapPtr sockets_map = NULL;
+virBitmapPtr *cores_maps = NULL;
 int sock_max = 0;
-cpu_set_t sock_map;
 int sock;
-cpu_set_t *core_maps = NULL;
 int core;
 size_t i;
 int siblings;
@@ -445,7 +434,9 @@ virNodeParseNode(const char *sysfs_prefix,
 goto cleanup;
 
 /* enumerate sockets in the node */
-CPU_ZERO(&sock_map);
+if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
+goto cleanup;
+
 while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
 if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
 continue;
@@ -462,7 +453,15 @@ virNodeParseNode(const char *sysfs_prefix,
 /* Parse socket */
 if ((sock = virNodeParseSocket(node, arch, cpu)) < 0)
 goto cleanup;
-CPU_SET(sock, &sock_map);
+if (sock > ID_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Socket %d can't be handled (max socket is %d)"),
+   sock, ID_MAX);
+goto cleanup;
+}
+
+if (virBitmapSetBit(sockets_map, sock) < 0)
+goto cleanup;
 
 if (sock > sock_max)
 sock_max = sock;
@@ -473,12 +472,13 @@ virNodeParseNode(const char *sysfs_prefix,
 
 sock_max++;
 
-/* allocate cpu maps for each socket */
-if (VIR_ALLOC_N(core_maps, sock_max) < 0)
+/* allocate cores maps for each socket */
+if (VIR_ALLOC_N(cores_maps, sock_max) < 0)
 goto cleanup;
 
 for (i = 0; i < sock_max; i++)
-CPU_ZERO(&core_maps[i]);
+if (!(cores_maps[i] = virBitmapNew(ID_MAX + 1)))
+goto cleanup;
 
 /* iterate over all CPU's in the node */
 rewinddir(cpudir);
@@ -502,7 +502,7 @@ virNodeParseNode(const char *sysfs_prefix,
 /* Parse socket */
 if ((sock = virNodeParseSocket(node, arch, cpu)) < 0)
 goto cleanup;
-if (!CPU_ISSET(sock, &sock_map)) {
+if (!virBitmapIsBitSet(sockets_map, sock)) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("CPU socket topology has changed"));
 goto cleanup;
@@ -515,8 +515,15 @@ virNodeParseNode(const char *sysfs_prefix,
 } else {
 core = virNodeGetCpuValue(node, cpu, "topology/core_id", 0);
 }
+if (core > ID_MAX) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Core %d can't be handled (max core is %d)"),
+   core, ID_MAX);
+goto cleanup;
+}
 
-CPU_SET(core, &core_maps[sock]);
+if (virBitmapSetBit(cores_maps[sock], core) < 0)
+goto cleanup;
 
 if (!(siblings = virNodeCountThreadSiblings(node, cpu)))
 goto cleanup;
@@ -529,13 +536,13 @@ virNodeParseNode(const char *sysfs_prefix,
 goto cleanup;
 
 /* finalize the returned data */
-*sockets = CPU_COUNT(&sock_map);
+*sockets = virBitmapCountBits(sockets_map);
 
 for (i = 0; i < sock_max; i++) {
-if (!CPU_ISSET(i, &sock_map))
+if (!virBitmapIsBitSet(sockets_map, i))

[libvirt] [PATCH v2 04/10] nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap()

[Andrea Bolognani]

If the cpu/present file is not available, we assume that the kernel
is too old to support non-consecutive CPU ids and return a bitmap
with all the bits set to represent this fact. This assumption is
already exploited in nodeGetCPUCount().

This means users of this API can expect the information to always
be available unless an error has occurred, and no longer need to
treat the NULL return value as a special case.

The error message has been updated as well.
---
 src/nodeinfo.c | 43 +--
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index a7a5d98..ceb517a 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -441,6 +441,8 @@ virNodeParseNode(const char *sysfs_prefix,
 }
 
 present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
+if (!present_cpumap)
+goto cleanup;
 
 /* enumerate sockets in the node */
 CPU_ZERO(&sock_map);
@@ -448,7 +450,7 @@ virNodeParseNode(const char *sysfs_prefix,
 if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
 continue;
 
-if (present_cpumap && !(virBitmapIsBitSet(present_cpumap, cpu)))
+if (!virBitmapIsBitSet(present_cpumap, cpu))
 continue;
 
 if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
@@ -484,7 +486,7 @@ virNodeParseNode(const char *sysfs_prefix,
 if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
 continue;
 
-if (present_cpumap && !(virBitmapIsBitSet(present_cpumap, cpu)))
+if (!virBitmapIsBitSet(present_cpumap, cpu))
 continue;
 
 if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
@@ -1284,27 +1286,40 @@ nodeGetCPUCount(const char *sysfs_prefix 
ATTRIBUTE_UNUSED)
 }
 
 virBitmapPtr
-nodeGetPresentCPUBitmap(const char *sysfs_prefix)
+nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
 {
-int max_present;
 #ifdef __linux__
+virBitmapPtr present_cpus = NULL;
 char *present_path = NULL;
-virBitmapPtr bitmap = NULL;
-#endif
+int npresent_cpus;
 
-if ((max_present = nodeGetCPUCount(sysfs_prefix)) < 0)
-return NULL;
+if ((npresent_cpus = nodeGetCPUCount(sysfs_prefix)) < 0)
+goto cleanup;
 
-#ifdef __linux__
 if (!(present_path = linuxGetCPUPresentPath(sysfs_prefix)))
-return NULL;
-if (virFileExists(present_path))
-bitmap = linuxParseCPUmap(max_present, present_path);
+goto cleanup;
+
+/* If the cpu/present file is available, parse it and exit */
+if (virFileExists(present_path)) {
+present_cpus = linuxParseCPUmap(npresent_cpus, present_path);
+goto cleanup;
+}
+
+/* If the file is not available, we can assume that the kernel is
+ * too old to support non-consecutive CPU ids and just mark all
+ * possible CPUs as present */
+if (!(present_cpus = virBitmapNew(npresent_cpus)))
+goto cleanup;
+
+virBitmapSetAll(present_cpus);
+
+ cleanup:
 VIR_FREE(present_path);
-return bitmap;
+
+return present_cpus;
 #endif
 virReportError(VIR_ERR_NO_SUPPORT, "%s",
-   _("non-continuous host cpu numbers not implemented on this 
platform"));
+   _("node present CPU map not implemented on this platform"));
 return NULL;
 }
 
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 00/10] nodeinfo: Various cleanups

[Andrea Bolognani]

This should take care of Peter's remarks, except the ones
that I've addressed separately.

Andrea Bolognani (10):
  nodeinfo: Introduce linuxGetCPUGlobalPath()
  nodeinfo: Introduce linuxGetCPUOnlinePath()
  nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()
  nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap()
  nodeinfo: Remove out parameter from nodeGetCPUBitmap()
  nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()
  nodeinfo: Phase out cpu_set_t usage
  nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
  nodeinfo: Use a bitmap to keep track of node CPUs
  nodeinfo: Calculate present and online CPUs only once

 src/libvirt_private.syms |   2 +-
 src/nodeinfo.c   | 214 +--
 src/nodeinfo.h   |   2 +-
 3 files changed, 136 insertions(+), 82 deletions(-)

-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 05/10] nodeinfo: Remove out parameter from nodeGetCPUBitmap()

[Andrea Bolognani]

Not all users of this API will need the size of the returned
bitmap; those who do can simply call virBitmapSize() themselves.
---
 src/nodeinfo.c | 12 +---
 src/nodeinfo.h |  2 +-
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index ceb517a..69c15fc 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -1324,8 +1324,7 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix 
ATTRIBUTE_UNUSED)
 }
 
 virBitmapPtr
-nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
- int *max_id ATTRIBUTE_UNUSED)
+nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
 {
 #ifdef __linux__
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
@@ -1363,8 +1362,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix 
ATTRIBUTE_UNUSED,
 ignore_value(virBitmapSetBit(cpumap, i));
 }
 }
-if (max_id && cpumap)
-*max_id = present;
+
  cleanup:
 VIR_FREE(online_path);
 VIR_FREE(cpudir);
@@ -1685,7 +1683,6 @@ nodeGetCPUMap(const char *sysfs_prefix,
   unsigned int flags)
 {
 virBitmapPtr cpus = NULL;
-int maxpresent;
 int ret = -1;
 int dummy;
 
@@ -1694,7 +1691,7 @@ nodeGetCPUMap(const char *sysfs_prefix,
 if (!cpumap && !online)
 return nodeGetCPUCount(sysfs_prefix);
 
-if (!(cpus = nodeGetCPUBitmap(sysfs_prefix, &maxpresent)))
+if (!(cpus = nodeGetCPUBitmap(sysfs_prefix)))
 goto cleanup;
 
 if (cpumap && virBitmapToData(cpus, cpumap, &dummy) < 0)
@@ -1702,7 +1699,8 @@ nodeGetCPUMap(const char *sysfs_prefix,
 if (online)
 *online = virBitmapCountBits(cpus);
 
-ret = maxpresent;
+ret = virBitmapSize(cpus);
+
  cleanup:
 if (ret < 0 && cpumap)
 VIR_FREE(*cpumap);
diff --git a/src/nodeinfo.h b/src/nodeinfo.h
index 4f983c2..02af9c5 100644
--- a/src/nodeinfo.h
+++ b/src/nodeinfo.h
@@ -45,7 +45,7 @@ int nodeGetMemory(unsigned long long *mem,
   unsigned long long *freeMem);
 
 virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
-virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int *max_id);
+virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix);
 int nodeGetCPUCount(const char *sysfs_prefix);
 
 int nodeGetMemoryParameters(virTypedParameterPtr params,
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 02/10] nodeinfo: Introduce linuxGetCPUOnlinePath()

[Andrea Bolognani]

---
 src/nodeinfo.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index b09a4fd..5459cc6 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -976,6 +976,12 @@ linuxGetCPUPresentPath(const char *sysfs_prefix)
 return linuxGetCPUGlobalPath(sysfs_prefix, "present");
 }
 
+static char *
+linuxGetCPUOnlinePath(const char *sysfs_prefix)
+{
+return linuxGetCPUGlobalPath(sysfs_prefix, "online");
+}
+
 /* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
 static int
 linuxParseCPUmax(const char *path)
@@ -1316,7 +1322,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix 
ATTRIBUTE_UNUSED,
 if (present < 0)
 return NULL;
 
-if (virAsprintf(&online_path, "%s/cpu/online", prefix) < 0)
+if (!(online_path = linuxGetCPUOnlinePath(sysfs_prefix)))
 return NULL;
 if (virFileExists(online_path)) {
 cpumap = linuxParseCPUmap(present, online_path);
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 08/10] nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node

[Andrea Bolognani]

No need to look up the online status of each CPU separately when we
can get all the information in one go.
---
 src/nodeinfo.c | 16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 7d0e6b0..c97dece 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -409,6 +409,7 @@ virNodeParseNode(const char *sysfs_prefix,
 DIR *cpudir = NULL;
 struct dirent *cpudirent = NULL;
 virBitmapPtr present_cpumap = NULL;
+virBitmapPtr online_cpus_map = NULL;
 virBitmapPtr sockets_map = NULL;
 virBitmapPtr *cores_maps = NULL;
 int sock_max = 0;
@@ -417,7 +418,6 @@ virNodeParseNode(const char *sysfs_prefix,
 size_t i;
 int siblings;
 unsigned int cpu;
-int online;
 int direrr;
 
 *threads = 0;
@@ -432,6 +432,9 @@ virNodeParseNode(const char *sysfs_prefix,
 present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
 if (!present_cpumap)
 goto cleanup;
+online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
+if (!online_cpus_map)
+goto cleanup;
 
 /* enumerate sockets in the node */
 if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
@@ -444,10 +447,7 @@ virNodeParseNode(const char *sysfs_prefix,
 if (!virBitmapIsBitSet(present_cpumap, cpu))
 continue;
 
-if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
-goto cleanup;
-
-if (!online)
+if (!virBitmapIsBitSet(online_cpus_map, cpu))
 continue;
 
 /* Parse socket */
@@ -489,10 +489,7 @@ virNodeParseNode(const char *sysfs_prefix,
 if (!virBitmapIsBitSet(present_cpumap, cpu))
 continue;
 
-if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
-goto cleanup;
-
-if (!online) {
+if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
 (*offline)++;
 continue;
 }
@@ -560,6 +557,7 @@ virNodeParseNode(const char *sysfs_prefix,
 virBitmapFree(cores_maps[i]);
 VIR_FREE(cores_maps);
 virBitmapFree(sockets_map);
+virBitmapFree(online_cpus_map);
 virBitmapFree(present_cpumap);
 
 return ret;
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 03/10] nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()

[Andrea Bolognani]

The original name was confusing because the function returns the number
of CPUs, not the maximum CPU id. The comment above the function has
been updated to reflect this.

No behavioral changes.
---
 src/nodeinfo.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 5459cc6..a7a5d98 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -982,9 +982,10 @@ linuxGetCPUOnlinePath(const char *sysfs_prefix)
 return linuxGetCPUGlobalPath(sysfs_prefix, "online");
 }
 
-/* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
+/* Determine the number of CPUs (maximum CPU id + 1) from a file containing
+ * a list of CPU ids, like the Linux sysfs cpu/present file */
 static int
-linuxParseCPUmax(const char *path)
+linuxParseCPUCount(const char *path)
 {
 char *str = NULL;
 char *tmp;
@@ -1246,7 +1247,7 @@ nodeGetCPUCount(const char *sysfs_prefix ATTRIBUTE_UNUSED)
 return -1;
 
 if (virFileExists(present_path)) {
-ncpu = linuxParseCPUmax(present_path);
+ncpu = linuxParseCPUCount(present_path);
 goto cleanup;
 }
 
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH v2 10/10] nodeinfo: Calculate present and online CPUs only once

[Andrea Bolognani]

Move the calls to the respective functions from virNodeParseNode(),
which is executed once for every NUMA node, to
linuxNodeInfoCPUPopulate(), which is executed just once per host.
---
 src/nodeinfo.c | 53 +++--
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 2328a86..31095a4 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -390,12 +390,15 @@ virNodeParseSocket(const char *dir,
 /* parses a node entry, returning number of processors in the node and
  * filling arguments */
 static int
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
-ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)
-ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7)
-virNodeParseNode(const char *sysfs_prefix,
- const char *node,
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
+ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(8)
+ATTRIBUTE_NONNULL(9)
+virNodeParseNode(const char *node,
  virArch arch,
+ virBitmapPtr present_cpus_map,
+ int npresent_cpus,
+ virBitmapPtr online_cpus_map,
  int *sockets,
  int *cores,
  int *threads,
@@ -408,12 +411,9 @@ virNodeParseNode(const char *sysfs_prefix,
 int processors = 0;
 DIR *cpudir = NULL;
 struct dirent *cpudirent = NULL;
-virBitmapPtr present_cpumap = NULL;
-virBitmapPtr online_cpus_map = NULL;
 virBitmapPtr node_cpus_map = NULL;
 virBitmapPtr sockets_map = NULL;
 virBitmapPtr *cores_maps = NULL;
-int npresent_cpus;
 int sock_max = 0;
 int sock;
 int core;
@@ -431,15 +431,6 @@ virNodeParseNode(const char *sysfs_prefix,
 goto cleanup;
 }
 
-present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
-if (!present_cpumap)
-goto cleanup;
-online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
-if (!online_cpus_map)
-goto cleanup;
-
-npresent_cpus = virBitmapSize(present_cpumap);
-
 /* Keep track of the CPUs that belong to the current node */
 if (!(node_cpus_map = virBitmapNew(npresent_cpus)))
 goto cleanup;
@@ -452,7 +443,7 @@ virNodeParseNode(const char *sysfs_prefix,
 if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
 continue;
 
-if (!virBitmapIsBitSet(present_cpumap, cpu))
+if (!virBitmapIsBitSet(present_cpus_map, cpu))
 continue;
 
 /* Mark this CPU as part of the current node */
@@ -565,8 +556,6 @@ virNodeParseNode(const char *sysfs_prefix,
 VIR_FREE(cores_maps);
 virBitmapFree(sockets_map);
 virBitmapFree(node_cpus_map);
-virBitmapFree(online_cpus_map);
-virBitmapFree(present_cpumap);
 
 return ret;
 }
@@ -578,10 +567,13 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
  virNodeInfoPtr nodeinfo)
 {
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
+virBitmapPtr present_cpus_map = NULL;
+virBitmapPtr online_cpus_map = NULL;
 char line[1024];
 DIR *nodedir = NULL;
 struct dirent *nodedirent = NULL;
 int cpus, cores, socks, threads, offline = 0;
+int npresent_cpus;
 unsigned int node;
 int ret = -1;
 char *sysfs_nodedir = NULL;
@@ -669,6 +661,17 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
 }
 }
 
+/* Get information about what CPUs are present in the host and what
+ * CPUs are online, so that we don't have to so for each node */
+present_cpus_map = nodeGetPresentCPUBitmap(sysfs_prefix);
+if (!present_cpus_map)
+goto cleanup;
+online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix);
+if (!online_cpus_map)
+goto cleanup;
+
+npresent_cpus = virBitmapSize(present_cpus_map);
+
 /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the
  * core, node, socket, thread and topology information from /sys
  */
@@ -690,7 +693,9 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
 prefix, nodedirent->d_name) < 0)
 goto cleanup;
 
-if ((cpus = virNodeParseNode(sysfs_prefix, sysfs_cpudir, arch,
+if ((cpus = virNodeParseNode(sysfs_cpudir, arch,
+ present_cpus_map, npresent_cpus,
+ online_cpus_map,
  &socks, &cores,
  &threads, &offline)) < 0)
 goto cleanup;
@@ -721,7 +726,9 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
 if (virAsprintf(&sysfs_cpudir, "%s/cpu", prefix) < 0)
 goto cleanup;
 
-if ((cpus = virNodeParseNode(sysfs_prefix, sysfs_cpudir, arch,
+if ((cpus = virNodeParseNode(sysfs_cpudir, arch,
+ present_cpus_map, npresent_cpus,
+ online_cpus_map,
  &socks, &cores,
  

[libvirt] [PATCH v2 01/10] nodeinfo: Introduce linuxGetCPUGlobalPath()

[Andrea Bolognani]

This is just a more generic version of linuxGetCPUPresentPath(),
which is now implemented by calling the new function appropriately.
---
 src/nodeinfo.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 105d7ab..b09a4fd 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -958,16 +958,24 @@ linuxNodeGetMemoryStats(FILE *meminfo,
 }
 
 static char *
-linuxGetCPUPresentPath(const char *sysfs_prefix)
+linuxGetCPUGlobalPath(const char *sysfs_prefix,
+  const char *file)
 {
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
 char *path = NULL;
 
-if (virAsprintf(&path, "%s/cpu/present", prefix) < 0)
+if (virAsprintf(&path, "%s/cpu/%s", prefix, file) < 0)
 return NULL;
+
 return path;
 }
 
+static char *
+linuxGetCPUPresentPath(const char *sysfs_prefix)
+{
+return linuxGetCPUGlobalPath(sysfs_prefix, "present");
+}
+
 /* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
 static int
 linuxParseCPUmax(const char *path)
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 09/10] nodeinfo: Use a bitmap to keep track of node CPUs

[Andrea Bolognani]

On Mon, 2015-07-20 at 15:47 +0200, Peter Krempa wrote:
> 
> > +/* Iterate over all CPUs in the node, in ascending order */
> > +for (cpu = 0; cpu < npresent_cpus; cpu++) {
> >  
> > -if (!virBitmapIsBitSet(present_cpumap, cpu))
> > +/* Skip CPUs that are not part of the current node */
> > +if (!virBitmapIsBitSet(node_cpus_map, cpu))
> 
> Perhaps you can use virBitmapNextSetBit to simplify the iteration.
> 
> >  continue;
> >  
> >  if (!virBitmapIsBitSet(online_cpus_map, cpu)) {
> 
> ACK with or without the iteration modified.

I tried following your suggestion but I didn't like the
result very much, so I've left the loop as-is in v2.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/10] nodeinfo: Phase out cpu_set_t usage

[Andrea Bolognani]

On Mon, 2015-07-20 at 15:41 +0200, Peter Krempa wrote:
> 
> > +/* Biggest value we can expect to be used as either socket id
> > + * or core id. Bitmaps will need to be sized accordingly */
> > +const int ID_MAX = 4095;
> 
> I think this should be a more global setting. We have quite a few 
> places
> where we invent arbitrary maximum cpu counts. One of them is
> virProcessSetAffinity.

Definitely agreed. We should define such limits in a single
place and stick to them.

> Otherwise looks good to me, but I'd really want to avoid multiple
> definitions of the same maximum variable.

I've left the code unchanged in v2 because this looks like
a task that would require quite a bit of research, and I'd
prefer if that didn't block an otherwise ACKed series which
in turn is a requirement of another series I've posted.

So I'm going to look into it and remove duplicate
definitions in a follow-up patch, if you're okay with that.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [libvirt-glib PATCHv5 7/7] gobject: Add wrapper for virNetworkGetDHCPLeases

[Zeeshan Ali (Khattak)]

On Thu, Jul 16, 2015 at 10:11 AM, Christophe Fergeau
 wrote:
> On Mon, Jul 13, 2015 at 02:45:21PM +0100, Zeeshan Ali (Khattak) wrote:
>> On Fri, Jul 10, 2015 at 5:04 PM, Christophe Fergeau  
>> wrote:
>> > Patches of yours broke the build, you have a strong opinion on the right 
>> > way
>> > to fix it, in such situations I usually go the extra mile to
>> > convince others that it's the best way :) That's why I'm a bit surprised
>> > this drags for so long with no real attempt at finding some common
>> > ground.
>>
>> I gave you an easy way out of this dragging discussion and even
>> promised to implement either of the solutions you want me to. You're
>> still not happy so I'll just bump the dependencies now. Feel free to
>> implement ugly hack solution. I'm out here..
>
> Thanks a lot for pushing an unreviewed patch after not wanting to go
> through proper patch discussion (ie do a bit of research in order to
> address the concerns which were raised rather than making up excuses for
> not doing it), that's appreciated!

I did address all your arguments and you still insist I did not. I
have no idea how I can possibly fix that.

FWIW, the patch I pushed only fixes a bug: libvirt-glib was requiring
a version of libvirt that it wasn't checking for in configure stage.

> After 10 minutes looking around, Ubuntu 14.04 LTS has libvirt 1.2.2 and
> SLES 12 has 1.2.5, both have long support cycles and a too old libvirt.
> Apart from these, supported Fedoras, latest Debian stable, opensuse 13.2
> and EL7.1 all have new enough libvirt.
> With this data in mind, and unless people from the impacted distros
> speak up, raising the requirement is probably reasonable enough.

And all that data is completely irrelevant for the reason I mentioned
again and again. Please point to at least one actual example of any
distro wanting to upgrade libvirt-glib to latest while wanting to keep
an year old release of libvirt and you'll have a point.

-- 
Regards,

Zeeshan Ali (Khattak)

Befriend GNOME: http://www.gnome.org/friends/

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] Load nbd module before running qemu-nbd

[Cédric Bosdonnat]

So far qemu-nbd is run even if the nbd kernel module isn't loaded. This
leads to errors when the user starts his lxc container while libvirt
could easily load the nbd module automatically.
---
 src/util/virfile.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 61f6e4d..76f1b7a 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -63,6 +63,7 @@
 #include "vircommand.h"
 #include "virerror.h"
 #include "virfile.h"
+#include "virkmod.h"
 #include "virlog.h"
 #include "virprocess.h"
 #include "virstring.h"
@@ -745,6 +746,7 @@ int virFileLoopDeviceAssociate(const char *file,
 
 
 # define SYSFS_BLOCK_DIR "/sys/block"
+# define NBD_DRIVER "nbd"
 
 
 static int
@@ -811,18 +813,42 @@ virFileNBDDeviceFindUnused(void)
 return ret;
 }
 
+static bool
+virFileNBDLoadDriver(void)
+{
+if (virKModIsBlacklisted(NBD_DRIVER)) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to load nbd module: "
+ "administratively prohibited"));
+return false;
+} else {
+char *errbuf = NULL;
+
+if ((errbuf = virKModLoad(NBD_DRIVER, true))) {
+VIR_FREE(errbuf);
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Failed to load nbd module"));
+return false;
+}
+VIR_FREE(errbuf);
+}
+return true;
+}
 
 int virFileNBDDeviceAssociate(const char *file,
   virStorageFileFormat fmt,
   bool readonly,
   char **dev)
 {
-char *nbddev;
+char *nbddev = NULL;
 char *qemunbd = NULL;
 virCommandPtr cmd = NULL;
 int ret = -1;
 const char *fmtstr = NULL;
 
+if (!virFileNBDLoadDriver())
+goto cleanup;
+
 if (!(nbddev = virFileNBDDeviceFindUnused()))
 goto cleanup;
 
-- 
2.1.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] network: add an option to make dns public

[Cedric Bosdonnat]

On Mon, 2015-07-20 at 16:25 +0200, Peter Krempa wrote:
> On Mon, Jul 20, 2015 at 11:29:15 +0200, Cédric Bosdonnat wrote:
> > In some use cases we don't want the virtual network's DNS to only
> > listen to the vnet interface. Adding a publiclyAccessible attribute
> > to the dns element in the configuration allows the DNS to listen to
> > all interfaces.
> 
> Would you please elaborate on the use cases where this would be useful?
> Libvirt networks shouldn't really be used for configuring dnsmasq for
> other purposes than for virtual machines where it's desired that the
> instances are separated.

This has been detailed in the previous mail thread, see here:
https://www.redhat.com/archives/libvir-list/2015-June/msg00781.html
and here:
https://www.redhat.com/archives/libvir-list/2015-June/msg00813.html

The feature has been requested by people using libvirt as a testing
infrastructure for cloud setups with vlans on top of the libvirt-defined
network. Maybe I should describe the use case in the commit log to avoid
the question being raised again and again.

--
Cedric

> > 
> > It simply disables the bind-dynamic option of dnsmasq for the network.
> > ---
> > 
> >  This patch is v2 for this one:
> >  https://www.redhat.com/archives/libvir-list/2015-June/msg00018.html
> > 
> >  Diff to v1:
> >* Use bind-interface if public DNS is requested
> >* Add more tests
> >* Write out the public value in the format function
> >* Fixed the rng
> >* Renamed the attribute to public: shouldn't mislead users
> > 
> >  I tested this patch with several configurations of running networks.
> >  The only thing I noted though is that the user may need to adapt the system
> >  dnsmasq to avoid address:port conflicts... but hey, when one uses such a
> >  hacky feature of the libvirt network, he needs to take care of the rest ;)
> > 
> 
> This paragraph emphasises that it doesn't sound like a good thing to do.
> 
> NACK unless you will persuade me with a good enough use case.
> 
> Peter
> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [Xen-devel] [PATCH 1/3] libxl: fix ref counting of libxlMigrationDstArgs

[Olaf Hering]

On Thu, Jul 16, Jim Fehlig wrote:

> @@ -448,6 +438,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>  virObjectUnref(socks[i]);
>  }
>  VIR_FREE(socks);
> +virObjectUnref(args);

This is now below the 'error' label, so args has to be initialized.

[  149s] libxl/libxl_migration.c: In function 'libxlDomainMigrationPrepare':
[  149s] libxl/libxl_migration.c:463:19: warning: 'args' may be used 
uninitialized in this function [-Wmaybe-uninitialized]
[  149s]  virObjectUnref(args);
[  149s]^

Olaf

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] rpc: RH1026137: Fix slow volume download (virsh vol-download)

[Ossi Herrala]

> On Sat, Jun 06, 2015 at 07:36:48PM +, Ossi Herrala wrote:
> 
> Sorry to miss this mail, it got buried somehow and I haven't got to it
> until now since nobody pinged it.  Sorry for the long wait then.
> 

No worries and thank you for taking time to review my patch. See new
patch attached as well as comments on the memory usage below.

The patch is tested on latest master (e46791e003444ce825feaf5bb2a16f778ee951e5).


> The only thing I would mention wrt to how it works after this patch is
> that it will consume some memory that's not needed, precisely
> (sizeof(struct iovec) + sizeof(void *)) * unreadMBs.  It might be
> worth it to do:
> 
>  memmove(st->incomingVec, st->incomingVec + st->readVec,
>  st->writeVec - st->readVec);
>  VIR_SHRINK_N(st->incomingVec, st->readVec);
>  st->writeVec -= st->readVec;
>  st->readVec = 0;
> 
> Apart from that it's definitely *way* better approach.  What do you
> think of such modification?  This would go either instead
> 'st->readVec++', but rather at the end of this function, so it's not
> done after each MB read.
> 

I totally agree and implemented freeing of the memory in the new
patch:

  if (st->readVec >= 16) {
memmove(st->incomingVec, st->incomingVec + st->readVec,
sizeof(*st->incomingVec)*(st->writeVec - st->readVec));
VIR_SHRINK_N(st->incomingVec, st->writeVec, st->readVec);
VIR_DEBUG("shrink removed %zu, left %zu", st->readVec, st->writeVec);
st->readVec = 0;
  }

now it only frees memory in chunks of 16 to avoid doing memmove() all
the time. The iovec is 16 bytes (in 64 bit Linux) so this frees 256
bytes at a time and in my testing usually everything or almost
everything that is allocated.


Thanks again for taking time to review and commit, and I hope the new
patch below is formatted ok.


>From 2ae95c31568eb800c1c6df3641a8ecbdc95bf268 Mon Sep 17 00:00:00 2001
From: Ossi Herrala 
Date: Mon, 20 Jul 2015 12:44:32 +
Subject: [PATCH] rpc: Fix slow volume download (virsh vol-download)

Use I/O vector (iovec) instead of one huge memory buffer as suggested
in https://bugzilla.redhat.com/show_bug.cgi?id=1026137#c7. This avoids
doing memmove() to big buffers and performance doesn't degrade if
source (virNetClientStreamQueuePacket()) is faster than sink
(virNetClientStreamRecvPacket()).

Resolves: http://bugzilla.redhat.com/1026137
---
 src/rpc/virnetclientstream.c |  152 +++---
 1 files changed, 99 insertions(+), 53 deletions(-)

diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
index b428f4b..1cc9002 100644
--- a/src/rpc/virnetclientstream.c
+++ b/src/rpc/virnetclientstream.c
@@ -49,9 +49,9 @@ struct _virNetClientStream {
  * time by stopping consuming any incoming data
  * off the socket
  */
-char *incoming;
-size_t incomingOffset;
-size_t incomingLength;
+struct iovec *incomingVec; /* I/O Vector to hold data */
+size_t writeVec;   /* Vectors produced */
+size_t readVec;/* Vectors consumed */
 bool incomingEOF;
 
 virNetClientStreamEventCallback cb;
@@ -86,9 +86,9 @@ virNetClientStreamEventTimerUpdate(virNetClientStreamPtr st)
 if (!st->cb)
 return;
 
-VIR_DEBUG("Check timer offset=%zu %d", st->incomingOffset, st->cbEvents);
+VIR_DEBUG("Check timer readVec %zu writeVec %zu %d", st->readVec, 
st->writeVec, st->cbEvents);
 
-if (((st->incomingOffset || st->incomingEOF) &&
+if st->readVec < st->writeVec) || st->incomingEOF) &&
  (st->cbEvents & VIR_STREAM_EVENT_READABLE)) ||
 (st->cbEvents & VIR_STREAM_EVENT_WRITABLE)) {
 VIR_DEBUG("Enabling event timer");
@@ -110,13 +110,14 @@ virNetClientStreamEventTimer(int timer ATTRIBUTE_UNUSED, 
void *opaque)
 
 if (st->cb &&
 (st->cbEvents & VIR_STREAM_EVENT_READABLE) &&
-(st->incomingOffset || st->incomingEOF))
+((st->readVec < st->writeVec) || st->incomingEOF))
 events |= VIR_STREAM_EVENT_READABLE;
 if (st->cb &&
 (st->cbEvents & VIR_STREAM_EVENT_WRITABLE))
 events |= VIR_STREAM_EVENT_WRITABLE;
 
-VIR_DEBUG("Got Timer dispatch %d %d offset=%zu", events, st->cbEvents, 
st->incomingOffset);
+VIR_DEBUG("Got Timer dispatch %d %d readVec %zu writeVec %zu", events, 
st->cbEvents,
+  st->readVec, st->writeVec);
 if (events) {
 virNetClientStreamEventCallback cb = st->cb;
 void *cbOpaque = st->cbOpaque;
@@ -161,7 +162,7 @@ void virNetClientStreamDispose(void *obj)
 virNetClientStreamPtr st = obj;
 
 virResetError(&st->err);
-VIR_FREE(st->incoming);
+VIR_FREE(st->incomingVec);
 virObjectUnref(st->prog);
 }
 
@@ -265,38 +266,50 @@ int virNetClientStreamQueuePacket(virNetClientStreamPtr 
st,
   virNetMessagePtr msg)
 {
 int ret = -1;
-size_t need;
+struct iovec iov;
+char *base;
+size_t piece, pieces, length, offset = 0, size = 1024*1024;
 
 v

Re: [libvirt] [PATCH 05/10] nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()

[Andrea Bolognani]

On Mon, 2015-07-20 at 16:18 +0200, Peter Krempa wrote:
> On Mon, Jul 20, 2015 at 16:07:42 +0200, Andrea Bolognani wrote:
> > On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
> > > 
> > > > -virBitmapPtr nodeGetPresentCPUBitmap(const char 
> > > > *sysfs_prefix);
> > > > -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int 
> > > > *max_id);
> > > > +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix,
> > > > + int *size);
> > > > +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix,
> > > > +  int *size);
> > > 
> > > I'd prefer something like "ncpus" or maxcpu rather than size. For
> > > getting size virBitmapSize() is totally apropriate.
> > 
> > I've used "size" on purpose, because I didn't want people to
> > mistake that for a count of online or present CPUs: it's the
> > size of the returned bitmap, same value you'd get if you
> > called virBitmapSize() on it.
> 
> I thin the 'max_id' or perhaps 'max_cpu_id' were better. Otherwise 
> I'd
> stay with calling virBitmapSize. It doesn't then look like it's 
> adding
> any value on top of calling virBitmapSize directly and could actually 
> be
> optimized out.

Using "max_id" is wrong though, because the returned value is
the size of the bitmap: if you have 4 CPUs, it will return 4,
not 3 as the name "max_id" would suggest. 

Since virBitmapSize() does very little work anyway, I vote
for getting rid of the out parameter altogether.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] Qemu: add CMT support

[Daniel P. Berrange]

On Mon, Jul 20, 2015 at 01:50:54PM +, Ren, Qiaowei wrote:
> 
> > -Original Message-
> > From: Daniel P. Berrange [mailto:berra...@redhat.com]
> > Sent: Monday, July 20, 2015 5:32 PM
> > To: Ren, Qiaowei
> > Cc: libvir-list@redhat.com
> > Subject: Re: [libvirt] [PATCH 2/3] Qemu: add CMT support
> > 
> > On Sun, Jul 05, 2015 at 07:43:43PM +0800, Qiaowei Ren wrote:
> > > One RFC in
> > > https://www.redhat.com/archives/libvir-list/2015-June/msg01509.html
> > >
> > > CMT (Cache Monitoring Technology) can be used to measure the usage of
> > > cache by VM running on the host. This patch will extend the bulk stats
> > > API (virDomainListGetStats) to add this field. Applications based on
> > > libvirt can use this API to achieve cache usage of VM. Because CMT
> > > implementation in Linux kernel is based on perf mechanism, this patch
> > > will enable perf event for CMT when VM is created and disable it when
> > > VM is destroyed.
> > >
> > > Signed-off-by: Qiaowei Ren 
> > 
> > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index
> > > 4cfae03..8c678c9 100644
> > > --- a/src/qemu/qemu_driver.c
> > > +++ b/src/qemu/qemu_driver.c
> > > @@ -19320,6 +19320,53 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr
> > > driver,
> > >
> > >  #undef QEMU_ADD_COUNT_PARAM
> > >
> > > +static int
> > > +qemuDomainGetStatsCache(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> > > +virDomainObjPtr dom,
> > > +virDomainStatsRecordPtr record,
> > > +int *maxparams,
> > > +unsigned int privflags ATTRIBUTE_UNUSED)
> > 
> > So this is a method that is used to collect per-domain information
> > 
> > > +{
> > > +qemuDomainObjPrivatePtr priv = dom->privateData;
> > > +FILE *fd;
> > > +unsigned long long cache = 0;
> > > +int scaling_factor = 0;
> > > +
> > > +if (priv->cmt_fd <= 0)
> > > +return -1;
> > > +
> > > +if (read(priv->cmt_fd, &cache, sizeof(uint64_t)) < 0) {
> > > +virReportSystemError(errno, "%s",
> > > + _("Unable to read cache data"));
> > > +return -1;
> > > +}
> > > +
> > > +fd = fopen("/sys/devices/intel_cqm/events/llc_occupancy.scale", "r");
> > > +if (!fd) {
> > > +virReportSystemError(errno, "%s",
> > > + _("Unable to open CMT scale file"));
> > > +return -1;
> > > +}
> > > +if (fscanf(fd, "%d", &scaling_factor) != 1) {
> > > +virReportSystemError(errno, "%s",
> > > + _("Unable to read CMT scale file"));
> > > +VIR_FORCE_FCLOSE(fd);
> > > +return -1;
> > > +}
> > > +VIR_FORCE_FCLOSE(fd);
> > 
> > But this data you are reading is global to the entire host.
> > 
> 
> In fact this data is only for per-domain.  When the perf syscall is
> called to enable perf event for domain, the pid of that domain is
> passed.

Ah, I see - you rely on the open file descriptor to be associated
with the VM pid.


> > > -5122,6 +5199,15 @@ void qemuProcessStop(virQEMUDriverPtr driver,
> > >  virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort);
> > >  priv->nbdPort = 0;
> > >
> > > +/* Disable CMT */
> > > +if (priv->cmt_fd > 0) {
> > 
> > You can't rely on keeping an open file descriptor for the guest because 
> > libvirtd
> > may be restarted.
> > 
> 
> Sorry, I don't really get the meaning of this. You mean that when libvirtd
> is restarted, those resource which the domain opened should be closed,
> right?

No, when libvirtd is restarted, the domains must all continuing running
without loss of state. You open the FD when starting the guest, then
libvirtd is restarted, now someone wants to query the perf data. The
perf FD will not be open anymore because libvirtd was restarted. At
least you'd need to re-open the file descriptor when libvirtd starts
up again, for any running guest. I'm not really convinced we want to
keep the perf file descriptors open for all the domains for the entire
time they are running. Should really only open them when we actually
want to read the collected data.

> 
> > > +if (ioctl(priv->cmt_fd, PERF_EVENT_IOC_DISABLE) < 0) {
> > > +virReportSystemError(errno, "%s",
> > > + _("Unable to disable perf event for 
> > > CMT"));
> > > +}
> > > +VIR_FORCE_CLOSE(priv->cmt_fd);
> > > +}
> > > +
> > >  if (priv->agent) {
> > >  qemuAgentClose(priv->agent);
> > >  priv->agent = NULL;
> > 
> > 
> > Conceptually I think this approach to implementation is flawed. While you 
> > are
> > turning on/off the perf events for each QEMU process, the data collection 
> > does
> > not distinguish data from each QEMU process - the data reported is host 
> > wide.
> > So this really doesn't make much sense IMHO.
> > 
> 
> As mentioned above, the data reported is only for domain.
> 
> > I'm also wondering whether 

Re: [libvirt] [PATCH v2] network: add an option to make dns public

[Peter Krempa]

On Mon, Jul 20, 2015 at 11:29:15 +0200, Cédric Bosdonnat wrote:
> In some use cases we don't want the virtual network's DNS to only
> listen to the vnet interface. Adding a publiclyAccessible attribute
> to the dns element in the configuration allows the DNS to listen to
> all interfaces.

Would you please elaborate on the use cases where this would be useful?
Libvirt networks shouldn't really be used for configuring dnsmasq for
other purposes than for virtual machines where it's desired that the
instances are separated.

> 
> It simply disables the bind-dynamic option of dnsmasq for the network.
> ---
> 
>  This patch is v2 for this one:
>  https://www.redhat.com/archives/libvir-list/2015-June/msg00018.html
> 
>  Diff to v1:
>* Use bind-interface if public DNS is requested
>* Add more tests
>* Write out the public value in the format function
>* Fixed the rng
>* Renamed the attribute to public: shouldn't mislead users
> 
>  I tested this patch with several configurations of running networks.
>  The only thing I noted though is that the user may need to adapt the system
>  dnsmasq to avoid address:port conflicts... but hey, when one uses such a
>  hacky feature of the libvirt network, he needs to take care of the rest ;)
> 

This paragraph emphasises that it doesn't sound like a good thing to do.

NACK unless you will persuade me with a good enough use case.

Peter



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/10] nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()

[Peter Krempa]

On Mon, Jul 20, 2015 at 16:07:42 +0200, Andrea Bolognani wrote:
> On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
> > 
> > > -virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
> > > -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int 
> > > *max_id);
> > > +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix,
> > > + int *size);
> > > +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix,
> > > +  int *size);
> > 
> > I'd prefer something like "ncpus" or maxcpu rather than size. For
> > getting size virBitmapSize() is totally apropriate.
> 
> I've used "size" on purpose, because I didn't want people to
> mistake that for a count of online or present CPUs: it's the
> size of the returned bitmap, same value you'd get if you
> called virBitmapSize() on it.

I thin the 'max_id' or perhaps 'max_cpu_id' were better. Otherwise I'd
stay with calling virBitmapSize. It doesn't then look like it's adding
any value on top of calling virBitmapSize directly and could actually be
optimized out.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 10/10] nodeinfo: Calculate present and online CPUs only once

[Peter Krempa]

On Mon, Jul 20, 2015 at 15:59:02 +0200, Andrea Bolognani wrote:
> Move the calls to the respective functions from virNodeParseNode(),
> which is executed once for every NUMA node, to
> linuxNodeInfoCPUPopulate(), which is executed just once per host.
> ---
>  src/nodeinfo.c | 49 +
>  1 file changed, 29 insertions(+), 20 deletions(-)
> 

ACK, although rather than patching up nodeinfo.c we should really
refactor it since it's a terrible mess.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/10] nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()

[Andrea Bolognani]

On Mon, 2015-07-20 at 15:15 +0200, Peter Krempa wrote:
> 
> > -virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
> > -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int 
> > *max_id);
> > +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix,
> > + int *size);
> > +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix,
> > +  int *size);
> 
> I'd prefer something like "ncpus" or maxcpu rather than size. For
> getting size virBitmapSize() is totally apropriate.

I've used "size" on purpose, because I didn't want people to
mistake that for a count of online or present CPUs: it's the
size of the returned bitmap, same value you'd get if you
called virBitmapSize() on it.

Sounds reasonable?

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/10] nodeinfo: Various cleanups

[Andrea Bolognani]

On Mon, 2015-07-20 at 15:48 +0200, Peter Krempa wrote:
> 
> > Andrea Bolognani (10):
> >   nodeinfo: Introduce linuxGetCPUGlobalPath()
> >   nodeinfo: Introduce linuxGetCPUOnlinePath()
> >   nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()
> >   nodeinfo: Add old kernel compatibility to 
> > nodeGetPresentCPUBitmap()
> >   nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()
> >   nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()
> >   nodeinfo: Phase out cpu_set_t usage
> >   nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
> >   nodeinfo: Use a bitmap to keep track of node CPUs
> >   nodeinfo: Calculate present and online CPUs only once
> 
> Patch 10/10 is missing in the list.

Yeah, sorry about that. It's in now.

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 10/10] nodeinfo: Calculate present and online CPUs only once

[Andrea Bolognani]

Move the calls to the respective functions from virNodeParseNode(),
which is executed once for every NUMA node, to
linuxNodeInfoCPUPopulate(), which is executed just once per host.
---
 src/nodeinfo.c | 49 +
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index 503fcdd..0f72a25 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -390,12 +390,15 @@ virNodeParseSocket(const char *dir,
 /* parses a node entry, returning number of processors in the node and
  * filling arguments */
 static int
-ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
-ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)
-ATTRIBUTE_NONNULL(6) ATTRIBUTE_NONNULL(7)
-virNodeParseNode(const char *sysfs_prefix,
- const char *node,
+ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
+ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
+ATTRIBUTE_NONNULL(7) ATTRIBUTE_NONNULL(8)
+ATTRIBUTE_NONNULL(9)
+virNodeParseNode(const char *node,
  virArch arch,
+ virBitmapPtr present_cpus_map,
+ int npresent_cpus,
+ virBitmapPtr online_cpus_map,
  int *sockets,
  int *cores,
  int *threads,
@@ -408,12 +411,9 @@ virNodeParseNode(const char *sysfs_prefix,
 int processors = 0;
 DIR *cpudir = NULL;
 struct dirent *cpudirent = NULL;
-virBitmapPtr present_cpumap = NULL;
-virBitmapPtr online_cpus_map = NULL;
 virBitmapPtr node_cpus_map = NULL;
 virBitmapPtr sockets_map = NULL;
 virBitmapPtr *cores_maps = NULL;
-int npresent_cpus;
 int sock_max = 0;
 int sock;
 int core;
@@ -431,13 +431,6 @@ virNodeParseNode(const char *sysfs_prefix,
 goto cleanup;
 }
 
-present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, &npresent_cpus);
-if (!present_cpumap)
-goto cleanup;
-online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
-if (!online_cpus_map)
-goto cleanup;
-
 /* Keep track of the CPUs that belong to the current node */
 if (!(node_cpus_map = virBitmapNew(npresent_cpus)))
 goto cleanup;
@@ -450,7 +443,7 @@ virNodeParseNode(const char *sysfs_prefix,
 if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
 continue;
 
-if (!virBitmapIsBitSet(present_cpumap, cpu))
+if (!virBitmapIsBitSet(present_cpus_map, cpu))
 continue;
 
 /* Mark this CPU as part of the current node */
@@ -563,8 +556,6 @@ virNodeParseNode(const char *sysfs_prefix,
 VIR_FREE(cores_maps);
 virBitmapFree(sockets_map);
 virBitmapFree(node_cpus_map);
-virBitmapFree(online_cpus_map);
-virBitmapFree(present_cpumap);
 
 return ret;
 }
@@ -576,10 +567,13 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
  virNodeInfoPtr nodeinfo)
 {
 const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
+virBitmapPtr present_cpus_map = NULL;
+virBitmapPtr online_cpus_map = NULL;
 char line[1024];
 DIR *nodedir = NULL;
 struct dirent *nodedirent = NULL;
 int cpus, cores, socks, threads, offline = 0;
+int npresent_cpus;
 unsigned int node;
 int ret = -1;
 char *sysfs_nodedir = NULL;
@@ -667,6 +661,15 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
 }
 }
 
+/* Get information about what CPUs are present in the host and what
+ * CPUs are online, so that we don't have to so for each node */
+present_cpus_map = nodeGetPresentCPUBitmap(sysfs_prefix, &npresent_cpus);
+if (!present_cpus_map)
+goto cleanup;
+online_cpus_map = nodeGetOnlineCPUBitmap(sysfs_prefix, NULL);
+if (!online_cpus_map)
+goto cleanup;
+
 /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the
  * core, node, socket, thread and topology information from /sys
  */
@@ -688,7 +691,9 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
 prefix, nodedirent->d_name) < 0)
 goto cleanup;
 
-if ((cpus = virNodeParseNode(sysfs_prefix, sysfs_cpudir, arch,
+if ((cpus = virNodeParseNode(sysfs_cpudir, arch,
+ present_cpus_map, npresent_cpus,
+ online_cpus_map,
  &socks, &cores,
  &threads, &offline)) < 0)
 goto cleanup;
@@ -719,7 +724,9 @@ linuxNodeInfoCPUPopulate(const char *sysfs_prefix,
 if (virAsprintf(&sysfs_cpudir, "%s/cpu", prefix) < 0)
 goto cleanup;
 
-if ((cpus = virNodeParseNode(sysfs_prefix, sysfs_cpudir, arch,
+if ((cpus = virNodeParseNode(sysfs_cpudir, arch,
+ present_cpus_map, npresent_cpus,
+ online_cpus_map,
  &socks, &cores,
  &threads, &offline)) < 0)
 goto cleanup;
@

Re: [libvirt] [PATCH 0/3] nodeinfo: Various fixes

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:12:47 +0200, Andrea Bolognani wrote:
> This series fixes a bunch of issues currently affecting nodeinfo
> and related tests.
> 
> Andrea Bolognani (3):
>   tests: Restore links in deconfigured-cpus nodeinfo test
>   nodeinfo: Add nodeGetPresentCPUBitmap() to libvirt_private.syms
>   nodeinfo: Fix nodeGetCPUBitmap()'s fallback code path

ACK series.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] Qemu: add CMT support

[Ren, Qiaowei]

> -Original Message-
> From: Daniel P. Berrange [mailto:berra...@redhat.com]
> Sent: Monday, July 20, 2015 5:32 PM
> To: Ren, Qiaowei
> Cc: libvir-list@redhat.com
> Subject: Re: [libvirt] [PATCH 2/3] Qemu: add CMT support
> 
> On Sun, Jul 05, 2015 at 07:43:43PM +0800, Qiaowei Ren wrote:
> > One RFC in
> > https://www.redhat.com/archives/libvir-list/2015-June/msg01509.html
> >
> > CMT (Cache Monitoring Technology) can be used to measure the usage of
> > cache by VM running on the host. This patch will extend the bulk stats
> > API (virDomainListGetStats) to add this field. Applications based on
> > libvirt can use this API to achieve cache usage of VM. Because CMT
> > implementation in Linux kernel is based on perf mechanism, this patch
> > will enable perf event for CMT when VM is created and disable it when
> > VM is destroyed.
> >
> > Signed-off-by: Qiaowei Ren 
> 
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index
> > 4cfae03..8c678c9 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -19320,6 +19320,53 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr
> > driver,
> >
> >  #undef QEMU_ADD_COUNT_PARAM
> >
> > +static int
> > +qemuDomainGetStatsCache(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> > +virDomainObjPtr dom,
> > +virDomainStatsRecordPtr record,
> > +int *maxparams,
> > +unsigned int privflags ATTRIBUTE_UNUSED)
> 
> So this is a method that is used to collect per-domain information
> 
> > +{
> > +qemuDomainObjPrivatePtr priv = dom->privateData;
> > +FILE *fd;
> > +unsigned long long cache = 0;
> > +int scaling_factor = 0;
> > +
> > +if (priv->cmt_fd <= 0)
> > +return -1;
> > +
> > +if (read(priv->cmt_fd, &cache, sizeof(uint64_t)) < 0) {
> > +virReportSystemError(errno, "%s",
> > + _("Unable to read cache data"));
> > +return -1;
> > +}
> > +
> > +fd = fopen("/sys/devices/intel_cqm/events/llc_occupancy.scale", "r");
> > +if (!fd) {
> > +virReportSystemError(errno, "%s",
> > + _("Unable to open CMT scale file"));
> > +return -1;
> > +}
> > +if (fscanf(fd, "%d", &scaling_factor) != 1) {
> > +virReportSystemError(errno, "%s",
> > + _("Unable to read CMT scale file"));
> > +VIR_FORCE_FCLOSE(fd);
> > +return -1;
> > +}
> > +VIR_FORCE_FCLOSE(fd);
> 
> But this data you are reading is global to the entire host.
> 

In fact this data is only for per-domain.  When the perf syscall is called to 
enable perf event for domain, the pid of that domain is passed.

> > +
> > +cache *= scaling_factor;
> > +
> > +if (virTypedParamsAddULLong(&record->params,
> > +&record->nparams,
> > +maxparams,
> > +"cache.current",
> > +cache) < 0)
> > +return -1;
> > +
> > +return 0;
> > +}
> > +
> 
> 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index
> > ba84182..00b889d 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> 
> > +/*
> > + * Enable CMT(Cache Monitoring Technology) to measure the usage of
> > + * cache by VM running on the node.
> > + *
> > + * Because the hypervisor implement CMT support basedon perf
> > +mechanism,
> > + * we should enable perf event for CMT. The function 'sys_erf_event_open'
> > + * is perf syscall wrapper.
> > + */
> > +#ifdef __linux__
> > +static long sys_perf_event_open(struct perf_event_attr *hw_event,
> > +pid_t pid, int cpu, int group_fd,
> > +unsigned long flags) {
> > +return syscall(__NR_perf_event_open, hw_event, pid, cpu,
> > +group_fd, flags); } static int
> > +qemuCmtEnable(virDomainObjPtr vm) {
> > +qemuDomainObjPrivatePtr priv = vm->privateData;
> > +struct perf_event_attr cmt_attr;
> > +int event_type;
> > +FILE *fp;
> > +
> > +fp = fopen("/sys/devices/intel_cqm/type", "r");
> > +if (!fp) {
> > +virReportSystemError(errno, "%s",
> > + _("CMT is not available on this host"));
> > +return -1;
> > +}
> > +if (fscanf(fp, "%d", &event_type) != 1) {
> > +virReportSystemError(errno, "%s",
> > + _("Unable to read event type file."));
> > +VIR_FORCE_FCLOSE(fp);
> > +return -1;
> > +}
> > +VIR_FORCE_FCLOSE(fp);
> > +
> > +memset(&cmt_attr, 0, sizeof(struct perf_event_attr));
> > +cmt_attr.size = sizeof(struct perf_event_attr);
> > +cmt_attr.type = event_type;
> > +cmt_attr.config = 1;
> > +cmt_attr.inherit = 1;
> > +cmt_attr.disabled = 1;
> > +cmt_attr.enable_on_exec = 0;
> > +
> > +priv->cmt_fd = sys

Re: [libvirt] [PATCH] qemuProcessStart: Be tolerant to relabel errors for session mode

[Daniel P. Berrange]

On Wed, Jul 15, 2015 at 03:02:13PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1124841
> 
> When the daemon is running under unprivileged user, that is under
> qemu:///session, there are plenty of operations we can't do. What
> we can do is to go with best effort. One of such cases is
> relabeling domain resources (be it disks, sockets, regular files,
> etc.) during domain startup process. While we may successfully set
> DAC labels, we can be fairly certain that any attempt to change
> SELinux labels will fail. Therefore we should tolerate relabelling
> errors and just let qemu to try access the resources. If it fails,
> our error reporting system is strong enough to articulate the
> exact error to the user anyway.

Errr, isn't it entirely the opposite to what you say. Running as
an unprivileged user ID has no bearing on whether you are allowed
to set SELinux labels. If the user acount is unconfined_t it can
set any SELinux labels it wants. It will only fail if the libvird
process is confined in some way.  IMHO we shold not be ignoring
such failures.

What *will* fail is any attempt to set DAC labels, since you need
CAP_CHOWN capability, but we shouldn't have the DAC security
maanger running when in session mode.

> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1c0c734..58ed631 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn,
>  
>  VIR_DEBUG("Setting domain security labels");
>  if (virSecurityManagerSetAllLabel(driver->securityManager,
> -  vm->def, stdin_path) < 0)
> -goto cleanup;
> +  vm->def, stdin_path) < 0) {
> +/* Be tolerant to relabel errors if we are running unprivileged. */
> +if (virQEMUDriverIsPrivileged(driver))
> +goto cleanup;
> +else
> +VIR_DEBUG("Ignoring relabel errors for unprivileged daemon");
> +}

I really don't think we should do this here as it affects all
security managers.

What is the failure you are actually seeing without this ?  SElinux
label changes should be succeeding in session mode and we should not
even be applying DAC labels

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/4] cpu: Rename {powerpc, ppc} => ppc64 (filesystem)

[Andrea Bolognani]

The driver only supports VIR_ARCH_PPC64 and VIR_ARCH_PPC64LE.

Just shuffling files around and updating the build system
accordingly. No functional changes.
---
 po/POTFILES.in   |   2 +-
 src/Makefile.am  |   5 +-
 src/cpu/cpu.c|   2 +-
 src/cpu/cpu.h|   2 +-
 src/cpu/cpu_powerpc.c| 711 ---
 src/cpu/cpu_powerpc.h|  32 ---
 src/cpu/cpu_ppc64.c  | 711 +++
 src/cpu/cpu_ppc64.h  |  32 +++
 src/cpu/cpu_ppc64_data.h |  33 +++
 src/cpu/cpu_ppc_data.h   |  33 ---
 10 files changed, 782 insertions(+), 781 deletions(-)
 delete mode 100644 src/cpu/cpu_powerpc.c
 delete mode 100644 src/cpu/cpu_powerpc.h
 create mode 100644 src/cpu/cpu_ppc64.c
 create mode 100644 src/cpu/cpu_ppc64.h
 create mode 100644 src/cpu/cpu_ppc64_data.h
 delete mode 100644 src/cpu/cpu_ppc_data.h

diff --git a/po/POTFILES.in b/po/POTFILES.in
index a75f5ae..c58a7c1 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -39,7 +39,7 @@ src/conf/virchrdev.c
 src/cpu/cpu.c
 src/cpu/cpu_generic.c
 src/cpu/cpu_map.c
-src/cpu/cpu_powerpc.c
+src/cpu/cpu_ppc64.c
 src/cpu/cpu_x86.c
 src/driver.c
 src/esx/esx_driver.c
diff --git a/src/Makefile.am b/src/Makefile.am
index 7338ab9..c4d49a5 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1008,8 +1008,9 @@ CPU_SOURCES = 
\
cpu/cpu_s390.h cpu/cpu_s390.c   \
cpu/cpu_arm.h cpu/cpu_arm.c \
cpu/cpu_aarch64.h cpu/cpu_aarch64.c \
-   cpu/cpu_map.h cpu/cpu_map.c cpu/cpu_powerpc.h   \
-   cpu/cpu_powerpc.c cpu/cpu_ppc_data.h
+   cpu/cpu_ppc64.h cpu/cpu_ppc64.c \
+   cpu/cpu_ppc64_data.h\
+   cpu/cpu_map.h cpu/cpu_map.c
 
 VMX_SOURCES =  \
vmx/vmx.c vmx/vmx.h
diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 9e67ddd..2e34f81 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -29,7 +29,7 @@
 #include "cpu.h"
 #include "cpu_map.h"
 #include "cpu_x86.h"
-#include "cpu_powerpc.h"
+#include "cpu_ppc64.h"
 #include "cpu_s390.h"
 #include "cpu_arm.h"
 #include "cpu_aarch64.h"
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index 09e9538..c0371cd 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -29,7 +29,7 @@
 # include "virarch.h"
 # include "conf/cpu_conf.h"
 # include "cpu_x86_data.h"
-# include "cpu_ppc_data.h"
+# include "cpu_ppc64_data.h"
 
 
 typedef struct _virCPUData virCPUData;
diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c
deleted file mode 100644
index 733a0cd..000
--- a/src/cpu/cpu_powerpc.c
+++ /dev/null
@@ -1,711 +0,0 @@
-/*
- * cpu_powerpc.c: CPU driver for PowerPC CPUs
- *
- * Copyright (C) 2013 Red Hat, Inc.
- * Copyright (C) IBM Corporation, 2010
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library.  If not, see
- * .
- *
- * Authors:
- *  Anton Blanchard 
- *  Prerna Saxena 
- *  Li Zhang 
- */
-
-#include 
-#include 
-
-#include "virlog.h"
-#include "viralloc.h"
-#include "cpu.h"
-#include "virstring.h"
-#include "cpu_map.h"
-#include "virbuffer.h"
-
-#define VIR_FROM_THIS VIR_FROM_CPU
-
-VIR_LOG_INIT("cpu.cpu_powerpc");
-
-static const virArch archs[] = { VIR_ARCH_PPC64, VIR_ARCH_PPC64LE };
-
-struct ppc_vendor {
-char *name;
-struct ppc_vendor *next;
-};
-
-struct ppc_model {
-char *name;
-const struct ppc_vendor *vendor;
-struct cpuPPCData data;
-struct ppc_model *next;
-};
-
-struct ppc_map {
-struct ppc_vendor *vendors;
-struct ppc_model *models;
-};
-
-
-static void
-ppcModelFree(struct ppc_model *model)
-{
-if (model == NULL)
-return;
-
-VIR_FREE(model->name);
-VIR_FREE(model);
-}
-
-static struct ppc_model *
-ppcModelFind(const struct ppc_map *map,
- const char *name)
-{
-struct ppc_model *model;
-
-model = map->models;
-while (model != NULL) {
-if (STREQ(model->name, name))
-return model;
-
-model = model->next;
-}
-
-return NULL;
-}
-
-static struct ppc_model *
-ppcModelFindPVR(const struct ppc_map *map,
-uint32_t pvr)
-{
-struct ppc_model *model;
-
-model = map->models;
-while (mo

[libvirt] [PATCH 0/4] cpu: Rename {powerpc,ppc} => ppc64

[Andrea Bolognani]

The naming of files and symbols belonging to the ppc64 CPU
driver was all over the place: this series brings
inner-peace-inducing consistency to that corner of libvirt
via a series of straightforward string replacements.

More substantial changes coming next.

Andrea Bolognani (4):
  cpu: Rename {powerpc,ppc} => ppc64 (filesystem)
  cpu: Rename {powerpc,ppc} => ppc64 (exported symbols)
  cpu: Rename {powerpc,ppc} => ppc64 (internal symbols)
  cpu: Indentation changes in the ppc64 driver

 po/POTFILES.in   |   2 +-
 src/Makefile.am  |   5 +-
 src/cpu/cpu.c|   4 +-
 src/cpu/cpu.h|   4 +-
 src/cpu/cpu_powerpc.c| 711 --
 src/cpu/cpu_powerpc.h|  32 ---
 src/cpu/cpu_ppc64.c  | 712 +++
 src/cpu/cpu_ppc64.h  |  32 +++
 src/cpu/cpu_ppc64_data.h |  33 +++
 src/cpu/cpu_ppc_data.h   |  33 ---
 10 files changed, 785 insertions(+), 783 deletions(-)
 delete mode 100644 src/cpu/cpu_powerpc.c
 delete mode 100644 src/cpu/cpu_powerpc.h
 create mode 100644 src/cpu/cpu_ppc64.c
 create mode 100644 src/cpu/cpu_ppc64.h
 create mode 100644 src/cpu/cpu_ppc64_data.h
 delete mode 100644 src/cpu/cpu_ppc_data.h

-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 00/10] nodeinfo: Various cleanups

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:19 +0200, Andrea Bolognani wrote:
> Note: this series is to be applied on top of the
> 
>   [PATCH 00/03] nodeinfo: Various fixes
> 
> series I've posted at the same time.
> 
> A bunch of improvements and cleanups that make the nodeinfo
> code a bit nicer, more streamlined and less redundant, hopefully
> not just to my eyes.
> 
> Andrea Bolognani (10):
>   nodeinfo: Introduce linuxGetCPUGlobalPath()
>   nodeinfo: Introduce linuxGetCPUOnlinePath()
>   nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()
>   nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap()
>   nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()
>   nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()
>   nodeinfo: Phase out cpu_set_t usage
>   nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node
>   nodeinfo: Use a bitmap to keep track of node CPUs
>   nodeinfo: Calculate present and online CPUs only once

Patch 10/10 is missing in the list.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/4] cpu: Rename {powerpc, ppc} => ppc64 (exported symbols)

[Andrea Bolognani]

Only the symbols exported by the driver have been updated;
the driver implementation itself still uses the old names
internally.

No functional changes.
---
 src/cpu/cpu.c|  2 +-
 src/cpu/cpu.h|  2 +-
 src/cpu/cpu_ppc64.c  | 18 +-
 src/cpu/cpu_ppc64.h  | 10 +-
 src/cpu/cpu_ppc64_data.h | 10 +-
 5 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
index 2e34f81..731df26 100644
--- a/src/cpu/cpu.c
+++ b/src/cpu/cpu.c
@@ -44,7 +44,7 @@ VIR_LOG_INIT("cpu.cpu");
 
 static struct cpuArchDriver *drivers[] = {
 &cpuDriverX86,
-&cpuDriverPowerPC,
+&cpuDriverPPC64,
 &cpuDriverS390,
 &cpuDriverArm,
 &cpuDriverAARCH64,
diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
index c0371cd..49d4226 100644
--- a/src/cpu/cpu.h
+++ b/src/cpu/cpu.h
@@ -38,7 +38,7 @@ struct _virCPUData {
 virArch arch;
 union {
 virCPUx86Data *x86;
-struct cpuPPCData ppc;
+struct cpuPPC64Data ppc64;
 /* generic driver needs no data */
 } data;
 };
diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 7866bdd..bec4bf8 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -1,5 +1,5 @@
 /*
- * cpu_ppc64.c: CPU driver for PowerPC CPUs
+ * cpu_ppc64.c: CPU driver for 64-bit PowerPC CPUs
  *
  * Copyright (C) 2013 Red Hat, Inc.
  * Copyright (C) IBM Corporation, 2010
@@ -36,7 +36,7 @@
 
 #define VIR_FROM_THIS VIR_FROM_CPU
 
-VIR_LOG_INIT("cpu.cpu_powerpc");
+VIR_LOG_INIT("cpu.cpu_ppc64");
 
 static const virArch archs[] = { VIR_ARCH_PPC64, VIR_ARCH_PPC64LE };
 
@@ -48,7 +48,7 @@ struct ppc_vendor {
 struct ppc_model {
 char *name;
 const struct ppc_vendor *vendor;
-struct cpuPPCData data;
+struct cpuPPC64Data data;
 struct ppc_model *next;
 };
 
@@ -340,7 +340,7 @@ ppcLoadMap(void)
 }
 
 static virCPUDataPtr
-ppcMakeCPUData(virArch arch, struct cpuPPCData *data)
+ppcMakeCPUData(virArch arch, struct cpuPPC64Data *data)
 {
 virCPUDataPtr cpuData;
 
@@ -348,7 +348,7 @@ ppcMakeCPUData(virArch arch, struct cpuPPCData *data)
 return NULL;
 
 cpuData->arch = arch;
-cpuData->data.ppc = *data;
+cpuData->data.ppc64 = *data;
 data = NULL;
 
 return cpuData;
@@ -480,10 +480,10 @@ ppcDecode(virCPUDefPtr cpu,
 if (data == NULL || (map = ppcLoadMap()) == NULL)
 return -1;
 
-if (!(model = ppcModelFindPVR(map, data->data.ppc.pvr))) {
+if (!(model = ppcModelFindPVR(map, data->data.ppc64.pvr))) {
 virReportError(VIR_ERR_OPERATION_FAILED,
_("Cannot find CPU model with PVR 0x%08x"),
-   data->data.ppc.pvr);
+   data->data.ppc64.pvr);
 goto cleanup;
 }
 
@@ -529,7 +529,7 @@ ppcNodeData(virArch arch)
 
 #if defined(__powerpc__) || defined(__powerpc64__)
 asm("mfpvr %0"
-: "=r" (cpuData->data.ppc.pvr));
+: "=r" (cpuData->data.ppc64.pvr));
 #endif
 
 return cpuData;
@@ -694,7 +694,7 @@ ppcGetModels(char ***models)
 goto cleanup;
 }
 
-struct cpuArchDriver cpuDriverPowerPC = {
+struct cpuArchDriver cpuDriverPPC64 = {
 .name = "ppc64",
 .arch = archs,
 .narch = ARRAY_CARDINALITY(archs),
diff --git a/src/cpu/cpu_ppc64.h b/src/cpu/cpu_ppc64.h
index e9ef2be..a6c9659 100644
--- a/src/cpu/cpu_ppc64.h
+++ b/src/cpu/cpu_ppc64.h
@@ -1,5 +1,5 @@
 /*
- * cpu_ppc64.h: CPU driver for PowerPC CPUs
+ * cpu_ppc64.h: CPU driver for 64-bit PowerPC CPUs
  *
  * Copyright (C) Copyright (C) IBM Corporation, 2010
  *
@@ -22,11 +22,11 @@
  *  Prerna Saxena 
  */
 
-#ifndef __VIR_CPU_POWERPC_H__
-# define __VIR_CPU_POWERPC_H__
+#ifndef __VIR_CPU_PPC64_H__
+# define __VIR_CPU_PPC64_H__
 
 # include "cpu.h"
 
-extern struct cpuArchDriver cpuDriverPowerPC;
+extern struct cpuArchDriver cpuDriverPPC64;
 
-#endif /* __VIR_CPU_POWERPC_H__ */
+#endif /* __VIR_CPU_PPC64_H__ */
diff --git a/src/cpu/cpu_ppc64_data.h b/src/cpu/cpu_ppc64_data.h
index a70b099..45152de 100644
--- a/src/cpu/cpu_ppc64_data.h
+++ b/src/cpu/cpu_ppc64_data.h
@@ -1,5 +1,5 @@
 /*
- * cpu_ppc64_data.h: PowerPC specific CPU data
+ * cpu_ppc64_data.h: 64-bit PowerPC CPU specific data
  *
  * Copyright (C) 2012 IBM Corporation.
  *
@@ -21,13 +21,13 @@
  *  Li Zhang 
  */
 
-#ifndef __VIR_CPU_PPC_DATA_H__
-# define __VIR_CPU_PPC_DATA_H__
+#ifndef __VIR_CPU_PPC64_DATA_H__
+# define __VIR_CPU_PPC64_DATA_H__
 
 # include 
 
-struct cpuPPCData {
+struct cpuPPC64Data {
 uint32_t pvr;
 };
 
-#endif /* __VIR_CPU_PPC_DATA_H__ */
+#endif /* __VIR_CPU_PPC64_DATA_H__ */
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 4/4] cpu: Indentation changes in the ppc64 driver

[Andrea Bolognani]

---
 src/cpu/cpu_ppc64.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index 7a48903..c3a51fb 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -340,7 +340,8 @@ ppc64LoadMap(void)
 }
 
 static virCPUDataPtr
-ppc64MakeCPUData(virArch arch, struct cpuPPC64Data *data)
+ppc64MakeCPUData(virArch arch,
+ struct cpuPPC64Data *data)
 {
 virCPUDataPtr cpuData;
 
@@ -695,9 +696,9 @@ ppc64GetModels(char ***models)
 }
 
 struct cpuArchDriver cpuDriverPPC64 = {
-.name = "ppc64",
-.arch = archs,
-.narch = ARRAY_CARDINALITY(archs),
+.name   = "ppc64",
+.arch   = archs,
+.narch  = ARRAY_CARDINALITY(archs),
 .compare= ppc64Compare,
 .decode = ppc64Decode,
 .encode = NULL,
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 3/4] cpu: Rename {powerpc, ppc} => ppc64 (internal symbols)

[Andrea Bolognani]

Update the names of the symbols used internally by the driver.

No functional changes.
---
 src/cpu/cpu_ppc64.c | 250 ++--
 1 file changed, 125 insertions(+), 125 deletions(-)

diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
index bec4bf8..7a48903 100644
--- a/src/cpu/cpu_ppc64.c
+++ b/src/cpu/cpu_ppc64.c
@@ -40,26 +40,26 @@ VIR_LOG_INIT("cpu.cpu_ppc64");
 
 static const virArch archs[] = { VIR_ARCH_PPC64, VIR_ARCH_PPC64LE };
 
-struct ppc_vendor {
+struct ppc64_vendor {
 char *name;
-struct ppc_vendor *next;
+struct ppc64_vendor *next;
 };
 
-struct ppc_model {
+struct ppc64_model {
 char *name;
-const struct ppc_vendor *vendor;
+const struct ppc64_vendor *vendor;
 struct cpuPPC64Data data;
-struct ppc_model *next;
+struct ppc64_model *next;
 };
 
-struct ppc_map {
-struct ppc_vendor *vendors;
-struct ppc_model *models;
+struct ppc64_map {
+struct ppc64_vendor *vendors;
+struct ppc64_model *models;
 };
 
 
 static void
-ppcModelFree(struct ppc_model *model)
+ppc64ModelFree(struct ppc64_model *model)
 {
 if (model == NULL)
 return;
@@ -68,11 +68,11 @@ ppcModelFree(struct ppc_model *model)
 VIR_FREE(model);
 }
 
-static struct ppc_model *
-ppcModelFind(const struct ppc_map *map,
- const char *name)
+static struct ppc64_model *
+ppc64ModelFind(const struct ppc64_map *map,
+   const char *name)
 {
-struct ppc_model *model;
+struct ppc64_model *model;
 
 model = map->models;
 while (model != NULL) {
@@ -85,11 +85,11 @@ ppcModelFind(const struct ppc_map *map,
 return NULL;
 }
 
-static struct ppc_model *
-ppcModelFindPVR(const struct ppc_map *map,
-uint32_t pvr)
+static struct ppc64_model *
+ppc64ModelFindPVR(const struct ppc64_map *map,
+  uint32_t pvr)
 {
-struct ppc_model *model;
+struct ppc64_model *model;
 
 model = map->models;
 while (model != NULL) {
@@ -105,19 +105,19 @@ ppcModelFindPVR(const struct ppc_map *map,
  * If the exact CPU isn't found, return the nearest matching CPU generation
  */
 if (pvr & 0xul)
-return ppcModelFindPVR(map, (pvr & 0xul));
+return ppc64ModelFindPVR(map, (pvr & 0xul));
 
 return NULL;
 }
 
-static struct ppc_model *
-ppcModelCopy(const struct ppc_model *model)
+static struct ppc64_model *
+ppc64ModelCopy(const struct ppc64_model *model)
 {
-struct ppc_model *copy;
+struct ppc64_model *copy;
 
 if (VIR_ALLOC(copy) < 0 ||
 VIR_STRDUP(copy->name, model->name) < 0) {
-ppcModelFree(copy);
+ppc64ModelFree(copy);
 return NULL;
 }
 
@@ -127,11 +127,11 @@ ppcModelCopy(const struct ppc_model *model)
 return copy;
 }
 
-static struct ppc_vendor *
-ppcVendorFind(const struct ppc_map *map,
-  const char *name)
+static struct ppc64_vendor *
+ppc64VendorFind(const struct ppc64_map *map,
+const char *name)
 {
-struct ppc_vendor *vendor;
+struct ppc64_vendor *vendor;
 
 vendor = map->vendors;
 while (vendor) {
@@ -145,7 +145,7 @@ ppcVendorFind(const struct ppc_map *map,
 }
 
 static void
-ppcVendorFree(struct ppc_vendor *vendor)
+ppc64VendorFree(struct ppc64_vendor *vendor)
 {
 if (!vendor)
 return;
@@ -154,34 +154,34 @@ ppcVendorFree(struct ppc_vendor *vendor)
 VIR_FREE(vendor);
 }
 
-static struct ppc_model *
-ppcModelFromCPU(const virCPUDef *cpu,
-const struct ppc_map *map)
+static struct ppc64_model *
+ppc64ModelFromCPU(const virCPUDef *cpu,
+  const struct ppc64_map *map)
 {
-struct ppc_model *model = NULL;
+struct ppc64_model *model = NULL;
 
-if ((model = ppcModelFind(map, cpu->model)) == NULL) {
+if ((model = ppc64ModelFind(map, cpu->model)) == NULL) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unknown CPU model %s"), cpu->model);
 goto error;
 }
 
-if ((model = ppcModelCopy(model)) == NULL)
+if ((model = ppc64ModelCopy(model)) == NULL)
 goto error;
 
 return model;
 
  error:
-ppcModelFree(model);
+ppc64ModelFree(model);
 return NULL;
 }
 
 
 static int
-ppcVendorLoad(xmlXPathContextPtr ctxt,
-  struct ppc_map *map)
+ppc64VendorLoad(xmlXPathContextPtr ctxt,
+struct ppc64_map *map)
 {
-struct ppc_vendor *vendor = NULL;
+struct ppc64_vendor *vendor = NULL;
 
 if (VIR_ALLOC(vendor) < 0)
 return -1;
@@ -193,7 +193,7 @@ ppcVendorLoad(xmlXPathContextPtr ctxt,
 goto ignore;
 }
 
-if (ppcVendorFind(map, vendor->name)) {
+if (ppc64VendorFind(map, vendor->name)) {
 virReportError(VIR_ERR_INTERNAL_ERROR,
_("CPU vendor %s already defined"), vendor->name);
 goto ignore;
@@ -210,15 +210,15 @@ ppcVendorLoad(xmlXPathContextPtr ctxt,
 return 0;
 
  ignore:
-ppcVendorFree(vendor);
+p

Re: [libvirt] [PATCH 09/10] nodeinfo: Use a bitmap to keep track of node CPUs

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:28 +0200, Andrea Bolognani wrote:
> Keep track of what CPUs belong to the current node while walking
> through the sysfs node entry, so we don't need to do it a second
> time immediately afterwards.
> 
> This also allows us to loop through all CPUs that are part of a
> node in guaranteed ascending order, which is something that is
> required for some upcoming changes.
> ---
>  src/nodeinfo.c | 26 --
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 

...

> @@ -480,13 +490,11 @@ virNodeParseNode(const char *sysfs_prefix,
>  if (!(cores_maps[i] = virBitmapNew(ID_MAX + 1)))
>  goto cleanup;
>  
> -/* iterate over all CPU's in the node */
> -rewinddir(cpudir);
> -while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
> -if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
> -continue;
> +/* Iterate over all CPUs in the node, in ascending order */
> +for (cpu = 0; cpu < npresent_cpus; cpu++) {
>  
> -if (!virBitmapIsBitSet(present_cpumap, cpu))
> +/* Skip CPUs that are not part of the current node */
> +if (!virBitmapIsBitSet(node_cpus_map, cpu))

Perhaps you can use virBitmapNextSetBit to simplify the iteration.

>  continue;
>  
>  if (!virBitmapIsBitSet(online_cpus_map, cpu)) {

ACK with or without the iteration modified.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemuProcessStart: Be tolerant to relabel errors for session mode

[John Ferlan]

On 07/15/2015 09:02 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1124841
> 
> When the daemon is running under unprivileged user, that is under
> qemu:///session, there are plenty of operations we can't do. What
> we can do is to go with best effort. One of such cases is
> relabeling domain resources (be it disks, sockets, regular files,
> etc.) during domain startup process. While we may successfully set
> DAC labels, we can be fairly certain that any attempt to change
> SELinux labels will fail. Therefore we should tolerate relabelling
> errors and just let qemu to try access the resources. If it fails,
> our error reporting system is strong enough to articulate the
> exact error to the user anyway.
> 
> Signed-off-by: Michal Privoznik 
> ---
>  src/qemu/qemu_process.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 1c0c734..58ed631 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -4856,8 +4856,13 @@ int qemuProcessStart(virConnectPtr conn,
>  
>  VIR_DEBUG("Setting domain security labels");
>  if (virSecurityManagerSetAllLabel(driver->securityManager,
> -  vm->def, stdin_path) < 0)
> -goto cleanup;
> +  vm->def, stdin_path) < 0) {
> +/* Be tolerant to relabel errors if we are running unprivileged. */
> +if (virQEMUDriverIsPrivileged(driver))
> +goto cleanup;
> +else
> +VIR_DEBUG("Ignoring relabel errors for unprivileged daemon");

How about just

if (cond)
   goto

VIR_DEBUG(or WARN)
virResetLastError()

Otherwise, seems reasonable in principal, so ACK

John
> +}
>  
>  /* Security manager labeled all devices, therefore
>   * if any operation from now on fails and we goto cleanup,
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 08/10] nodeinfo: Use nodeGetOnlineCPUBitmap() when parsing node

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:27 +0200, Andrea Bolognani wrote:
> No need to look up the online status of each CPU separately when we
> can get all the information in one go.
> ---
>  src/nodeinfo.c | 16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 

ACK,

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 07/10] nodeinfo: Phase out cpu_set_t usage

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:26 +0200, Andrea Bolognani wrote:
> Swap out all instances of cpu_set_t and replace them with virBitmap,
> which some of the code was already using anyway.
> 
> The changes are pretty mechanical, with one notable exception: an
> assumption has been added on the max value we can run into while
> reading either socket_it or core_id.
> 
> While this specific assumption was not in place before, we were
> using cpu_set_t improperly by not making sure not to set any bit
> past CPU_SETSIZE or explicitly allocating bigger bitmaps; in fact
> the default size of a cpu_set_t, 1024, is way too low to run our
> testsuite, which includes core_id values in the 2000s.
> ---
>  src/nodeinfo.c | 65 
> ++
>  1 file changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 44983b8..61a3b33 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -30,7 +30,6 @@
>  #include 
>  #include 
>  #include 
> -#include 
>  #include "conf/domain_conf.h"
>  
>  #if defined(__FreeBSD__) || defined(__APPLE__)
> @@ -388,19 +387,6 @@ virNodeParseSocket(const char *dir,
>  return ret;
>  }
>  
> -# ifndef CPU_COUNT
> -static int
> -CPU_COUNT(cpu_set_t *set)
> -{
> -size_t i, count = 0;
> -
> -for (i = 0; i < CPU_SETSIZE; i++)
> -if (CPU_ISSET(i, set))
> -count++;
> -return count;
> -}
> -# endif /* !CPU_COUNT */
> -
>  /* parses a node entry, returning number of processors in the node and
>   * filling arguments */
>  static int
> @@ -415,15 +401,18 @@ virNodeParseNode(const char *sysfs_prefix,
>   int *threads,
>   int *offline)
>  {
> +/* Biggest value we can expect to be used as either socket id
> + * or core id. Bitmaps will need to be sized accordingly */
> +const int ID_MAX = 4095;

I think this should be a more global setting. We have quite a few places
where we invent arbitrary maximum cpu counts. One of them is
virProcessSetAffinity.

>  int ret = -1;
>  int processors = 0;
>  DIR *cpudir = NULL;
>  struct dirent *cpudirent = NULL;
>  virBitmapPtr present_cpumap = NULL;
> +virBitmapPtr sockets_map = NULL;
> +virBitmapPtr *cores_maps = NULL;
>  int sock_max = 0;
> -cpu_set_t sock_map;
>  int sock;
> -cpu_set_t *core_maps = NULL;
>  int core;
>  size_t i;
>  int siblings;
> @@ -445,7 +434,9 @@ virNodeParseNode(const char *sysfs_prefix,
>  goto cleanup;
>  
>  /* enumerate sockets in the node */
> -CPU_ZERO(&sock_map);
> +if (!(sockets_map = virBitmapNew(ID_MAX + 1)))
> +goto cleanup;
> +
>  while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) {
>  if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>  continue;
> @@ -462,7 +453,15 @@ virNodeParseNode(const char *sysfs_prefix,
>  /* Parse socket */
>  if ((sock = virNodeParseSocket(node, arch, cpu)) < 0)
>  goto cleanup;
> -CPU_SET(sock, &sock_map);
> +if (sock > ID_MAX) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("Socket %d can't be handled (max socket is 
> %d)"),
> +   sock, ID_MAX);
> +goto cleanup;
> +}
> +
> +if (virBitmapSetBit(sockets_map, sock) < 0)
> +goto cleanup;

Like many others, this code would benefit from auto-allocating bitmaps
rather than the static ones. Nothing to change though here.

>  
>  if (sock > sock_max)
>  sock_max = sock;

Otherwise looks good to me, but I'd really want to avoid multiple
definitions of the same maximum variable.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] unsupported configuration: unknown video model 'virtio' - virtio-vga

[Martin Kletzander]

On Mon, Jul 20, 2015 at 11:23:02AM +0200, poma wrote:


$ qemu-system-x86_64 ... -device virtio-vga


# lspci -d 1af4:1050 -knn
00:03.0 VGA compatible controller [0300]: Red Hat, Inc Device [1af4:1050] (rev 
01)
Subsystem: Red Hat, Inc Device [1af4:1100]
Kernel driver in use: virtio-pci
Kernel modules: virtio_pci


# dmesg | grep virtio
[1.727390] [drm] pci: virtio-vga detected
[1.729315] [drm] virtio vbuffers: 80 bufs, 192B each, 15kB total.
[2.023845] virtio_gpu virtio0: fb0: virtiodrmfb frame buffer device
[2.023846] virtio_gpu virtio0: registered panic notifier
[2.043135] [drm] Initialized virtio_gpu 0.0.1 0 on minor 0


# journalctl -b -u libvirtd.service -o cat
Starting Virtualization daemon...
Started Virtualization daemon.
libvirt version: 1.2.17, package: 1.fc23 (Fedora Project, 2015-07-14-18:18:48, 
buildvm-03.phx2.fedoraproject.org)
unsupported configuration: unknown video model 'virtio'



I'm guessing you are posting all these outputs from one machine,
right?  Then that doesn't make sense.  I think the error from libvirt
is because you have a domain with invalid XML in
/etc/libvirt/... where you should *NOT* touch it.

What's the output of the following command:

 grep -ri 'type=.virtio.' /etc/libvirt/



Soonish?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] rpc: Remove keepalive_required option

[Martin Kletzander]

Since its introduction in 2011 (particularly in commit f4324e329275),
the option doesn't work.  It just effectively disables all incoming
connections.  That's because the client private data that contain the
'keepalive_supported' boolean, are initialized to zeroes so the bool is
false and the only other place where the bool is used is when checking
whether the client supports keepalive.  Thus, according to the server,
no client supports keepalive.

Removing this instead of fixing it is better because a) apparently
nobody ever tried it since 2011 (4 years without one month) and b) we
cannot know whether the client supports keepalive until we get a ping or
pong keepalive packet.  And that won't happen untile after we dispatched
the ConnectOpen call.

Another two reasons would be c) the keepalive_required was tracked on
the server level, but keepalive_supported was in private data of the
client as well as the check that was made in the remote layer, thus
making all other instances of virNetServer miss this feature unless they
all implemented it for themselves and d) we can always add it back in
case there is a request and a use-case for it.

Signed-off-by: Martin Kletzander 
---
 daemon/libvirtd-config.c   |  4 
 daemon/libvirtd-config.h   |  2 --
 daemon/libvirtd.aug|  2 --
 daemon/libvirtd.c  |  2 --
 daemon/libvirtd.conf   |  7 --
 daemon/libvirtd.h  |  1 -
 daemon/remote.c|  8 +--
 daemon/test_libvirtd.aug.in|  2 --
 src/libvirt_remote.syms|  1 -
 src/locking/lock_daemon.c  |  2 +-
 src/lxc/lxc_controller.c   |  2 +-
 src/rpc/virnetserver.c | 25 +-
 src/rpc/virnetserver.h |  3 ---
 .../virnetdaemondata/output-data-admin-nomdns.json |  2 --
 .../virnetdaemondata/output-data-anon-clients.json |  1 -
 .../output-data-initial-nomdns.json|  1 -
 tests/virnetdaemondata/output-data-initial.json|  1 -
 tests/virnetdaemontest.c   |  2 +-
 18 files changed, 5 insertions(+), 63 deletions(-)

diff --git a/daemon/libvirtd-config.c b/daemon/libvirtd-config.c
index 10dcc423d2db..c31c8b2e9ab5 100644
--- a/daemon/libvirtd-config.c
+++ b/daemon/libvirtd-config.c
@@ -292,7 +292,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)

 data->keepalive_interval = 5;
 data->keepalive_count = 5;
-data->keepalive_required = 0;

 data->admin_min_workers = 5;
 data->admin_max_workers = 20;
@@ -302,7 +301,6 @@ daemonConfigNew(bool privileged ATTRIBUTE_UNUSED)

 data->admin_keepalive_interval = 5;
 data->admin_keepalive_count = 5;
-data->admin_keepalive_required = 0;

 localhost = virGetHostname();
 if (localhost == NULL) {
@@ -471,11 +469,9 @@ daemonConfigLoadOptions(struct daemonConfig *data,

 GET_CONF_INT(conf, filename, keepalive_interval);
 GET_CONF_UINT(conf, filename, keepalive_count);
-GET_CONF_UINT(conf, filename, keepalive_required);

 GET_CONF_INT(conf, filename, admin_keepalive_interval);
 GET_CONF_UINT(conf, filename, admin_keepalive_count);
-GET_CONF_UINT(conf, filename, admin_keepalive_required);

 return 0;

diff --git a/daemon/libvirtd-config.h b/daemon/libvirtd-config.h
index 9cdae1a0cb59..3e1971d67f05 100644
--- a/daemon/libvirtd-config.h
+++ b/daemon/libvirtd-config.h
@@ -81,7 +81,6 @@ struct daemonConfig {

 int keepalive_interval;
 unsigned int keepalive_count;
-int keepalive_required;

 int admin_min_workers;
 int admin_max_workers;
@@ -91,7 +90,6 @@ struct daemonConfig {

 int admin_keepalive_interval;
 unsigned int admin_keepalive_count;
-int admin_keepalive_required;
 };


diff --git a/daemon/libvirtd.aug b/daemon/libvirtd.aug
index a70aa1dddf90..7c7992dd0568 100644
--- a/daemon/libvirtd.aug
+++ b/daemon/libvirtd.aug
@@ -79,11 +79,9 @@ module Libvirtd =

let keepalive_entry = int_entry "keepalive_interval"
| int_entry "keepalive_count"
-   | bool_entry "keepalive_required"

let admin_keepalive_entry = int_entry "admin_keepalive_interval"
  | int_entry "admin_keepalive_count"
- | bool_entry "admin_keepalive_required"

let misc_entry = str_entry "host_uuid"

diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 71db4a042c7f..250094bd21dd 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1389,7 +1389,6 @@ int main(int argc, char **argv) {
 config->max_anonymous_clients,
 config->keepalive_interval,
 config->keepalive_count,
-!!config

Re: [libvirt] [PATCH 06/10] nodeinfo: Rename nodeGetCPUBitmap() to nodeGetOnlineCPUBitmap()

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:25 +0200, Andrea Bolognani wrote:
> The new name makes it clear that the returned bitmap contains the
> information about which CPUs are online, not eg. which CPUs are
> present.
> 
> Change the name of the out parameter from max_id, which didn't
> reflect the actual value returned, to size. Update the error
> message as well.
> 
> No functional changes.
> ---
>  src/libvirt_private.syms |  2 +-
>  src/nodeinfo.c   | 12 ++--
>  src/nodeinfo.h   |  4 ++--
>  3 files changed, 9 insertions(+), 9 deletions(-)
> 


> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 7aecc8f..44983b8 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -1325,8 +1325,8 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix 
> ATTRIBUTE_UNUSED,
>  }
>  
>  virBitmapPtr
> -nodeGetCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> - int *max_id ATTRIBUTE_UNUSED)
> +nodeGetOnlineCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> +   int *size ATTRIBUTE_UNUSED)

Same comment for the second parameter name as in the previous patch.

ACK to the function name change.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 05/10] nodeinfo: Add out parameter to nodeGetPresentCPUBitmap()

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:24 +0200, Andrea Bolognani wrote:
> This aligns it with nodeGetCPUBitmap(), which already has a
> similar out parameters, and relieves users of this API from the
> need to call virBitmapSize() on the returned bitmap.
> ---
>  src/nodeinfo.c   | 8 ++--
>  src/nodeinfo.h   | 6 --
>  src/util/vircgroup.c | 4 +---
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 5aa0607..7aecc8f 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -440,7 +440,7 @@ virNodeParseNode(const char *sysfs_prefix,
>  goto cleanup;
>  }
>  
> -present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
> +present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix, NULL);
>  if (!present_cpumap)
>  goto cleanup;
>  
> @@ -1280,7 +1280,8 @@ nodeGetCPUCount(const char *sysfs_prefix 
> ATTRIBUTE_UNUSED)
>  }
>  
>  virBitmapPtr
> -nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
> +nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED,
> +int *size ATTRIBUTE_UNUSED)
>  {
>  #ifdef __linux__
>  virBitmapPtr present_cpus = NULL;
> @@ -1313,6 +1314,9 @@ nodeGetPresentCPUBitmap(const char *sysfs_prefix 
> ATTRIBUTE_UNUSED)
>   cleanup:
>  VIR_FREE(present_path);
>  
> +if (present_cpus && size)
> +*size = npresent_cpus;
> +
>  return present_cpus;
>  #endif
>  virReportError(VIR_ERR_NO_SUPPORT, "%s",
> diff --git a/src/nodeinfo.h b/src/nodeinfo.h
> index 4f983c2..e83db7b 100644
> --- a/src/nodeinfo.h
> +++ b/src/nodeinfo.h
> @@ -44,8 +44,10 @@ int nodeGetCellsFreeMemory(unsigned long long *freeMems,
>  int nodeGetMemory(unsigned long long *mem,
>unsigned long long *freeMem);
>  
> -virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix);
> -virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix, int *max_id);
> +virBitmapPtr nodeGetPresentCPUBitmap(const char *sysfs_prefix,
> + int *size);
> +virBitmapPtr nodeGetCPUBitmap(const char *sysfs_prefix,
> +  int *size);

I'd prefer something like "ncpus" or maxcpu rather than size. For
getting size virBitmapSize() is totally apropriate.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 04/10] nodeinfo: Add old kernel compatibility to nodeGetPresentCPUBitmap()

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:23 +0200, Andrea Bolognani wrote:
> If the cpu/present file is not available, we assume that the kernel
> is too old to support non-consecutive CPU ids and return a bitmap
> with all the bits set to represent this fact. This assumption is
> already exploited in nodeGetCPUCount().
> 
> This means users of this API can expect the information to always
> be available unless an error has occurred, and no longer need to
> treat the NULL return value as a special case.
> 
> The error message has been updated as well.
> ---
>  src/nodeinfo.c | 46 --
>  1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 52f5594..5aa0607 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -441,6 +441,8 @@ virNodeParseNode(const char *sysfs_prefix,
>  }
>  
>  present_cpumap = nodeGetPresentCPUBitmap(sysfs_prefix);
> +if (!present_cpumap)
> +goto cleanup;
>  
>  /* enumerate sockets in the node */
>  CPU_ZERO(&sock_map);
> @@ -448,7 +450,7 @@ virNodeParseNode(const char *sysfs_prefix,
>  if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>  continue;
>  
> -if (present_cpumap && !(virBitmapIsBitSet(present_cpumap, cpu)))
> +if (!virBitmapIsBitSet(present_cpumap, cpu))
>  continue;
>  
>  if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
> @@ -484,7 +486,7 @@ virNodeParseNode(const char *sysfs_prefix,
>  if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1)
>  continue;
>  
> -if (present_cpumap && !(virBitmapIsBitSet(present_cpumap, cpu)))
> +if (!virBitmapIsBitSet(present_cpumap, cpu))
>  continue;
>  
>  if ((online = virNodeGetCpuValue(node, cpu, "online", 1)) < 0)
> @@ -1278,27 +1280,43 @@ nodeGetCPUCount(const char *sysfs_prefix 
> ATTRIBUTE_UNUSED)
>  }
>  
>  virBitmapPtr
> -nodeGetPresentCPUBitmap(const char *sysfs_prefix)
> +nodeGetPresentCPUBitmap(const char *sysfs_prefix ATTRIBUTE_UNUSED)
>  {
> -int max_present;
>  #ifdef __linux__
> +virBitmapPtr present_cpus = NULL;
>  char *present_path = NULL;
> -virBitmapPtr bitmap = NULL;
> -#endif
> +int npresent_cpus;
> +int cpu;
>  
> -if ((max_present = nodeGetCPUCount(sysfs_prefix)) < 0)
> -return NULL;
> +if ((npresent_cpus = nodeGetCPUCount(sysfs_prefix)) < 0)
> +goto cleanup;
>  
> -#ifdef __linux__
>  if (!(present_path = linuxGetCPUPresentPath(sysfs_prefix)))
> -return NULL;
> -if (virFileExists(present_path))
> -bitmap = linuxParseCPUmap(max_present, present_path);
> +goto cleanup;
> +
> +/* If the cpu/present file is available, parse it and exit */
> +if (virFileExists(present_path)) {
> +present_cpus = linuxParseCPUmap(npresent_cpus, present_path);
> +goto cleanup;
> +}
> +
> +/* If the file is not available, we can assume that the kernel is
> + * too old to support non-consecutive CPU ids and just mark all
> + * possible CPUs as present */
> +if (!(present_cpus = virBitmapNew(npresent_cpus)))
> +goto cleanup;
> +
> +for (cpu = 0; cpu < npresent_cpus; cpu++)
> +if (virBitmapSetBit(present_cpus, cpu) < 0)

virBitmapSetAll();

> +goto cleanup;
> +
> + cleanup:
>  VIR_FREE(present_path);
> -return bitmap;
> +
> +return present_cpus;
>  #endif
>  virReportError(VIR_ERR_NO_SUPPORT, "%s",
> -   _("non-continuous host cpu numbers not implemented on 
> this platform"));
> +   _("node present CPU map not implemented on this 
> platform"));
>  return NULL;
>  }

ACK with changing to virBitmapSetAll. It makes nodeGetPresentCPUBitmap a
little less horrible than it used to be.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] tc ingress rule of VM B disappear when reboot VM A

[ychen]

hi:
when I use openstack devstack to test QOS, wired phenomenon appeared, 
I set qos ingress rule in tapB, but when I reboot tapA, the ingress rule of 
tapB automatically removed, but the egress rule is still exist.
Test enviroment:
Linux: ubuntu 14.04.1 LTS
kernel: 3.13.0-32-generic
libvirt: 1.2.2
openstack: havana


1. use nova to create vm A and vm B. for the configuration of the libvirt xml, 
see the last paragraph in the end.
2. use tc cmd to create qos rule for vm A and vm B
   tc qdisc add dev tap3d0d2c4a-0b ingress  //vmA
   tc qdisc add dev tap896d5066-69 ingress //vmB


3. then use cmd 
  "sudo virsh destory 142a08db-6e25-4a03-be13-7073104b0745 "  to first shutdown 
vm1
  then I see ingress rule of vmB disappeared :( 




configurations:
  vmA:---


  
  instance-0001
  142a08db-6e25-4a03-be13-7073104b0745
  524288
  524288
  1
  
/machine
  
  

  OpenStack Foundation
  OpenStack Nova
  2013.2.3
  5fd079ed-5bc3-45ed-8de5-8bf9b8533d82
  142a08db-6e25-4a03-be13-7073104b0745

  
  
hvm


  
  


  
  
  destroy
  restart
  destroy
  
/usr/bin/qemu-system-x86_64

  
  
  
  
  


  
  


  


  
  
  

  
  
  
  
  
  


  
  
  


  
  
  


  
  
  


  




  


  
  
  


  
  

  
  
libvirt-142a08db-6e25-4a03-be13-7073104b0745
libvirt-142a08db-6e25-4a03-be13-7073104b0745
  



 vmB:---

  instance-0002
  fbd69f7b-83f1-45fe-818b-2021d5cb2e61
  524288
  524288
  1
  
/machine
  
  

  OpenStack Foundation
  OpenStack Nova
  2013.2.3
  5fd079ed-5bc3-45ed-8de5-8bf9b8533d82
  fbd69f7b-83f1-45fe-818b-2021d5cb2e61

  
  
hvm


  
  


  
  
  destroy
  restart
  destroy
  
/usr/bin/qemu-system-x86_64

  
  
  
  
  


  
  


  


  
  
  

  
  
  
  
  
  


  
  
  


  
  
  


  
  
  


  




  


  
  
  


  
  

  
  
libvirt-fbd69f7b-83f1-45fe-818b-2021d5cb2e61
libvirt-fbd69f7b-83f1-45fe-818b-2021d5cb2e61
  


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] nodeinfo: fix build on FreeBSD

[Roman Bogorodskiy]

  Martin Kletzander wrote:

> On Fri, Jul 17, 2015 at 08:23:07PM +0300, Roman Bogorodskiy wrote:
> >Currently, build fails on FreeBSD with:
> >
> >  CC   libvirt_driver_la-nodeinfo.lo
> >nodeinfo.c:1941:56: error: use of undeclared identifier 'SYSFS_SYSTEM_PATH'
> >const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
> >   ^
> >1 error generated.
> >
> >This is caused by commit b97b3048 that added sysfs_prefix to
> >nodeCapsInitNUMA and used SYSFS_CPU_PATH.
> >
> >Fix it by unconditionally defining SYSFS_CPU_PATH instead of defining it
> >under #ifdef __linux__.
> >---
> > src/nodeinfo.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> 
> ACK, can't hurt, could've been considered under build-breaker rule.

Pushed, thanks!
I didn't forget about the build-breaker rule, however, there were no
rush and I was wondering if somebody'd suggest a better fix.

Roman Bogorodskiy

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/10] nodeinfo: Introduce linuxGetCPUOnlinePath()

[Andrea Bolognani]

On Mon, 2015-07-20 at 11:34 +0200, Peter Krempa wrote:
> 
> > @@ -973,6 +973,9 @@ linuxGetCPUGlobalPath(const char *sysfs_prefix,
> >  # define linuxGetCPUPresentPath(sysfs_prefix)\
> >  linuxGetCPUGlobalPath(sysfs_prefix, "present")
> >  
> > +# define linuxGetCPUOnlinePath(sysfs_prefix) \
> > +linuxGetCPUGlobalPath(sysfs_prefix, "online")
> 
> Either add a function ...
> 
> > +
> >  /* Determine the maximum cpu id from a Linux sysfs cpu/present 
> > file. */
> >  static int
> >  linuxParseCPUmax(const char *path)
> > @@ -1313,7 +1316,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix 
> > ATTRIBUTE_UNUSED,
> >  if (present < 0)
> >  return NULL;
> >  
> > -if (virAsprintf(&online_path, "%s/cpu/online", prefix) < 0)
> > +if (!(online_path = linuxGetCPUOnlinePath(sysfs_prefix)))
> 
> Or use the global helper here directly.

I'll send v2 converting both this and linuxGetCPUPresentPath()
to functions once you're done reviewing the series :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qxl: Fix new function name for spice-server library

[Martin Kletzander]

On Mon, Jul 20, 2015 at 09:43:23AM +0100, Frediano Ziglio wrote:

The new spice-server function to limit the number of monitors (0.12.6)
changed while development from spice_qxl_set_monitors_config_limit to
spice_qxl_max_monitors (accepted upstream).
By mistake I post patch with former name.
This patch fix the function name.

Signed-off-by: Frediano Ziglio 

---
hw/display/qxl.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

I tested again doing a clean build, unfortunately I did some mistake
and my tests worked.

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 4e5ff69..2288238 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -273,8 +273,7 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice 
*qxl, int replay)
} else {
#if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
if (qxl->max_outputs) {
-spice_qxl_set_monitors_config_limit(&qxl->ssd.qxl,
-qxl->max_outputs);
+spice_qxl_set_max_monitors(&qxl->ssd.qxl, qxl->max_outputs);
}
#endif
qxl->guest_monitors_config = qxl->ram->monitors_config;
--
2.1.0



Same as the fix I did in order for this to work with upstream spice.

ACK.  Weak, though, as I'm not a privileged one.

Martin


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] nodeinfo: fix build on FreeBSD

[Martin Kletzander]

On Fri, Jul 17, 2015 at 08:23:07PM +0300, Roman Bogorodskiy wrote:

Currently, build fails on FreeBSD with:

 CC   libvirt_driver_la-nodeinfo.lo
nodeinfo.c:1941:56: error: use of undeclared identifier 'SYSFS_SYSTEM_PATH'
   const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
  ^
1 error generated.

This is caused by commit b97b3048 that added sysfs_prefix to
nodeCapsInitNUMA and used SYSFS_CPU_PATH.

Fix it by unconditionally defining SYSFS_CPU_PATH instead of defining it
under #ifdef __linux__.
---
src/nodeinfo.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)



ACK, can't hurt, could've been considered under build-breaker rule.


diff --git a/src/nodeinfo.c b/src/nodeinfo.c
index c874fa6..d7d0223 100644
--- a/src/nodeinfo.c
+++ b/src/nodeinfo.c
@@ -59,6 +59,8 @@

VIR_LOG_INIT("nodeinfo");

+#define SYSFS_SYSTEM_PATH "/sys/devices/system"
+
#if defined(__FreeBSD__) || defined(__APPLE__)
static int
appleFreebsdNodeGetCPUCount(void)
@@ -283,7 +285,6 @@ freebsdNodeGetMemoryStats(virNodeMemoryStatsPtr params,

#ifdef __linux__
# define CPUINFO_PATH "/proc/cpuinfo"
-# define SYSFS_SYSTEM_PATH "/sys/devices/system"
# define PROCSTAT_PATH "/proc/stat"
# define MEMINFO_PATH "/proc/meminfo"
# define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm"
--
2.4.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 03/10] nodeinfo: Rename linuxParseCPUmax() to linuxParseCPUCount()

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:22 +0200, Andrea Bolognani wrote:
> The original name was confusing because the function returns the number
> of CPUs, not the maximum CPU id. The comment above the function has
> been updated to reflect this.
> 
> No functional changes.
> ---
>  src/nodeinfo.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

ACK


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: Reject updating unsupported disk information

[Martin Kletzander]

On Fri, Jul 17, 2015 at 04:21:29PM +0200, Michal Privoznik wrote:

On 10.07.2015 08:20, Martin Kletzander wrote:

If one calls update-device with information that is not updatable,
libvirt reports success even though no data were updated.  The example
used in the bug linked below uses updating device with 
which, in my opinion, is a valid thing to request from user's
perspective.  Mainly since we properly error out if user wants to update
such data on a network device for example.

And since there are many things that might happen (update-device on disk
basically knows just how to change removable media), check for what's
changing and moreover, since the function might be usable in other
drivers (updating only disk path is a valid possibility) let's abstract
it for any two disks.

We can't possibly check for everything since for many fields our code
does not properly differentiate between default and unspecified values.
Even though this could be changed, I don't feel like it's worth the
complexity so it's not the aim of this patch.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228
Signed-off-by: Martin Kletzander 
---

Notes:
v2:
 - Don't say 'NULL' when it should be 'unspecified'
 - Don't blindly copy field name into the error message, but use space
   instead of dot.  That way it looks the same as in the XML provided.
 - Check strings properly instead of addresses

 src/conf/domain_conf.c   | 133 +++
 src/conf/domain_conf.h   |   2 +
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   |   3 ++
 4 files changed, 139 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1f7862b00463..69e5df27c270 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5690,6 +5690,139 @@ virDomainDiskFindByBusAndDst(virDomainDefPtr def,
 return NULL;
 }

+
+/*
+ * Makes sure the @disk differs from @orig_disk only by the source
+ * path and nothing else.  Fields that are being checked and the
+ * information whether they are nullable (may not be specified) or is
+ * taken from the virDomainDiskDefFormat() code.
+ */
+bool
+virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
+   virDomainDiskDefPtr orig_disk)
+{


Nice work, but I think this should go somewhere into src/qemu/. I mean,
some o these fields are not changeable as a domain is running. But some
might be depending on the underlying hypervisor. For instance, xen might
be able to change serial number of a HDD. On the other hand, that might
be just a few corner cases I'm talking about, so it's up to you what you
prefer more.



Well, I'd prefer it to be here for now.  And that's also why I didn't
call the function virDomainDiskUpdatable() as that has nothing to do
with updating a disk, it's just a "coincidence" that qemu uses it for
that purpose ;)


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 02/10] nodeinfo: Introduce linuxGetCPUOnlinePath()

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:21 +0200, Andrea Bolognani wrote:
> ---
>  src/nodeinfo.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 64b12e6..7a12d54 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -973,6 +973,9 @@ linuxGetCPUGlobalPath(const char *sysfs_prefix,
>  # define linuxGetCPUPresentPath(sysfs_prefix)\
>  linuxGetCPUGlobalPath(sysfs_prefix, "present")
>  
> +# define linuxGetCPUOnlinePath(sysfs_prefix) \
> +linuxGetCPUGlobalPath(sysfs_prefix, "online")

Either add a function ...

> +
>  /* Determine the maximum cpu id from a Linux sysfs cpu/present file. */
>  static int
>  linuxParseCPUmax(const char *path)
> @@ -1313,7 +1316,7 @@ nodeGetCPUBitmap(const char *sysfs_prefix 
> ATTRIBUTE_UNUSED,
>  if (present < 0)
>  return NULL;
>  
> -if (virAsprintf(&online_path, "%s/cpu/online", prefix) < 0)
> +if (!(online_path = linuxGetCPUOnlinePath(sysfs_prefix)))

Or use the global helper here directly.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2] qemu: Reject updating unsupported disk information

[Martin Kletzander]

On Fri, Jul 17, 2015 at 04:58:46PM +0200, Michal Privoznik wrote:

On 17.07.2015 16:21, Michal Privoznik wrote:

On 10.07.2015 08:20, Martin Kletzander wrote:

If one calls update-device with information that is not updatable,
libvirt reports success even though no data were updated.  The example
used in the bug linked below uses updating device with 
which, in my opinion, is a valid thing to request from user's
perspective.  Mainly since we properly error out if user wants to update
such data on a network device for example.

And since there are many things that might happen (update-device on disk
basically knows just how to change removable media), check for what's
changing and moreover, since the function might be usable in other
drivers (updating only disk path is a valid possibility) let's abstract
it for any two disks.

We can't possibly check for everything since for many fields our code
does not properly differentiate between default and unspecified values.
Even though this could be changed, I don't feel like it's worth the
complexity so it's not the aim of this patch.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1007228
Signed-off-by: Martin Kletzander 
---

Notes:
v2:
 - Don't say 'NULL' when it should be 'unspecified'
 - Don't blindly copy field name into the error message, but use space
   instead of dot.  That way it looks the same as in the XML provided.
 - Check strings properly instead of addresses

 src/conf/domain_conf.c   | 133 +++
 src/conf/domain_conf.h   |   2 +
 src/libvirt_private.syms |   1 +
 src/qemu/qemu_driver.c   |   3 ++
 4 files changed, 139 insertions(+)







ACK


This was premature. You need to squash this in:

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 2448a37..6562157 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5781,7 +5781,7 @@ virDomainDiskDiffersSourceOnly(virDomainDiskDefPtr disk,
 "blkdeviotune size_iops_sec",
 true);

-if (disk->transient && STRNEQ(disk->transient, orig_disk->transient)) {
+if (disk->transient != orig_disk->transient) {


Actually, if you don't specify  for a disk that is already
transient, this will fail to update the media.  But you're right this
must not be string comparison.  I changed this check to:

 CHECK_EQ(transient, "transient", true);

Which is a bit more complicated, but shorter version of:

 if (disk->transient && !orig_disk->transient)
 ...


virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
   _("cannot modify field '%s' of the disk"),
   "transient");


disk->transient is a bool, not string.

Michal


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 01/10] nodeinfo: Introduce linuxGetCPUGlobalPath()

[Peter Krempa]

On Fri, Jul 17, 2015 at 18:13:20 +0200, Andrea Bolognani wrote:
> This is just a more generic version of linuxGetCPUPresentPath(),
> which is now implemented by calling the new function appropriately.
> ---
>  src/nodeinfo.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/nodeinfo.c b/src/nodeinfo.c
> index 105d7ab..64b12e6 100644
> --- a/src/nodeinfo.c
> +++ b/src/nodeinfo.c
> @@ -958,16 +958,21 @@ linuxNodeGetMemoryStats(FILE *meminfo,
>  }
>  
>  static char *
> -linuxGetCPUPresentPath(const char *sysfs_prefix)
> +linuxGetCPUGlobalPath(const char *sysfs_prefix,
> +  const char *file)
>  {
>  const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SYSTEM_PATH;
>  char *path = NULL;
>  
> -if (virAsprintf(&path, "%s/cpu/present", prefix) < 0)
> +if (virAsprintf(&path, "%s/cpu/%s", prefix, file) < 0)
>  return NULL;
> +
>  return path;
>  }
>  
> +# define linuxGetCPUPresentPath(sysfs_prefix)\
> +linuxGetCPUGlobalPath(sysfs_prefix, "present")

I'd rather see a wrapper function that adds the argument rather than a
macro.

ACK with that change.

Peter


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/3] Qemu: add CMT support

[Daniel P. Berrange]

On Sun, Jul 05, 2015 at 07:43:43PM +0800, Qiaowei Ren wrote:
> One RFC in
> https://www.redhat.com/archives/libvir-list/2015-June/msg01509.html
> 
> CMT (Cache Monitoring Technology) can be used to measure the
> usage of cache by VM running on the host. This patch will
> extend the bulk stats API (virDomainListGetStats) to add this
> field. Applications based on libvirt can use this API to achieve
> cache usage of VM. Because CMT implementation in Linux kernel
> is based on perf mechanism, this patch will enable perf event
> for CMT when VM is created and disable it when VM is destroyed.
> 
> Signed-off-by: Qiaowei Ren 

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4cfae03..8c678c9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -19320,6 +19320,53 @@ qemuDomainGetStatsBlock(virQEMUDriverPtr driver,
>  
>  #undef QEMU_ADD_COUNT_PARAM
>  
> +static int
> +qemuDomainGetStatsCache(virQEMUDriverPtr driver ATTRIBUTE_UNUSED,
> +virDomainObjPtr dom,
> +virDomainStatsRecordPtr record,
> +int *maxparams,
> +unsigned int privflags ATTRIBUTE_UNUSED)

So this is a method that is used to collect per-domain information

> +{
> +qemuDomainObjPrivatePtr priv = dom->privateData;
> +FILE *fd;
> +unsigned long long cache = 0;
> +int scaling_factor = 0;
> +
> +if (priv->cmt_fd <= 0)
> +return -1;
> +
> +if (read(priv->cmt_fd, &cache, sizeof(uint64_t)) < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to read cache data"));
> +return -1;
> +}
> +
> +fd = fopen("/sys/devices/intel_cqm/events/llc_occupancy.scale", "r");
> +if (!fd) {
> +virReportSystemError(errno, "%s",
> + _("Unable to open CMT scale file"));
> +return -1;
> +}
> +if (fscanf(fd, "%d", &scaling_factor) != 1) {
> +virReportSystemError(errno, "%s",
> + _("Unable to read CMT scale file"));
> +VIR_FORCE_FCLOSE(fd);
> +return -1;
> +}
> +VIR_FORCE_FCLOSE(fd);

But this data you are reading is global to the entire host.

> +
> +cache *= scaling_factor;
> +
> +if (virTypedParamsAddULLong(&record->params,
> +&record->nparams,
> +maxparams,
> +"cache.current",
> +cache) < 0)
> +return -1;
> +
> +return 0;
> +}
> +


> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index ba84182..00b889d 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c

> +/*
> + * Enable CMT(Cache Monitoring Technology) to measure the usage of
> + * cache by VM running on the node.
> + *
> + * Because the hypervisor implement CMT support basedon perf mechanism,
> + * we should enable perf event for CMT. The function 'sys_erf_event_open'
> + * is perf syscall wrapper.
> + */
> +#ifdef __linux__
> +static long sys_perf_event_open(struct perf_event_attr *hw_event,
> +pid_t pid, int cpu, int group_fd,
> +unsigned long flags)
> +{
> +return syscall(__NR_perf_event_open, hw_event, pid, cpu,
> +group_fd, flags);
> +}
> +static int qemuCmtEnable(virDomainObjPtr vm)
> +{
> +qemuDomainObjPrivatePtr priv = vm->privateData;
> +struct perf_event_attr cmt_attr;
> +int event_type;
> +FILE *fp;
> +
> +fp = fopen("/sys/devices/intel_cqm/type", "r");
> +if (!fp) {
> +virReportSystemError(errno, "%s",
> + _("CMT is not available on this host"));
> +return -1;
> +}
> +if (fscanf(fp, "%d", &event_type) != 1) {
> +virReportSystemError(errno, "%s",
> + _("Unable to read event type file."));
> +VIR_FORCE_FCLOSE(fp);
> +return -1;
> +}
> +VIR_FORCE_FCLOSE(fp);
> +
> +memset(&cmt_attr, 0, sizeof(struct perf_event_attr));
> +cmt_attr.size = sizeof(struct perf_event_attr);
> +cmt_attr.type = event_type;
> +cmt_attr.config = 1;
> +cmt_attr.inherit = 1;
> +cmt_attr.disabled = 1;
> +cmt_attr.enable_on_exec = 0;
> +
> +priv->cmt_fd = sys_perf_event_open(&cmt_attr, vm->pid, -1, -1, 0);
> +if (priv->cmt_fd < 0) {
> +virReportSystemError(errno,
> + _("Unable to open perf type=%d for pid=%d"),
> + event_type, vm->pid);
> +return -1;
> +}
> +
> +if (ioctl(priv->cmt_fd, PERF_EVENT_IOC_ENABLE) < 0) {
> +virReportSystemError(errno, "%s",
> + _("Unable to enable perf event for CMT"));
> +return -1;
> +}
> +
> +return 0;
> +}
> +#else
> +static int qemuCmtEnable(virDomainObjPtr vm)
> +{
> +virReportUnsupportedError();
> 

[libvirt] [PATCH v2] network: add an option to make dns public

[Cédric Bosdonnat]

In some use cases we don't want the virtual network's DNS to only
listen to the vnet interface. Adding a publiclyAccessible attribute
to the dns element in the configuration allows the DNS to listen to
all interfaces.

It simply disables the bind-dynamic option of dnsmasq for the network.
---

 This patch is v2 for this one:
 https://www.redhat.com/archives/libvir-list/2015-June/msg00018.html

 Diff to v1:
   * Use bind-interface if public DNS is requested
   * Add more tests
   * Write out the public value in the format function
   * Fixed the rng
   * Renamed the attribute to public: shouldn't mislead users

 I tested this patch with several configurations of running networks.
 The only thing I noted though is that the user may need to adapt the system
 dnsmasq to avoid address:port conflicts... but hey, when one uses such a
 hacky feature of the libvirt network, he needs to take care of the rest ;)

 docs/formatnetwork.html.in |  9 +++
 docs/schemas/network.rng   |  5 
 src/conf/network_conf.c| 28 --
 src/conf/network_conf.h|  1 +
 src/network/bridge_driver.c|  3 ++-
 .../nat-network-dns-not-public.conf| 15 
 .../nat-network-dns-not-public.xml | 15 
 .../nat-network-dns-public.conf| 15 
 .../networkxml2confdata/nat-network-dns-public.xml | 15 
 tests/networkxml2xmlin/nat-network-dns-public.xml  |  9 +++
 tests/networkxml2xmlout/nat-network-dns-public.xml | 11 +
 tests/networkxml2xmltest.c |  1 +
 12 files changed, 124 insertions(+), 3 deletions(-)
 create mode 100644 tests/networkxml2confdata/nat-network-dns-not-public.conf
 create mode 100644 tests/networkxml2confdata/nat-network-dns-not-public.xml
 create mode 100644 tests/networkxml2confdata/nat-network-dns-public.conf
 create mode 100644 tests/networkxml2confdata/nat-network-dns-public.xml
 create mode 100644 tests/networkxml2xmlin/nat-network-dns-public.xml
 create mode 100644 tests/networkxml2xmlout/nat-network-dns-public.xml

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in
index 6abed8f..0141d93 100644
--- a/docs/formatnetwork.html.in
+++ b/docs/formatnetwork.html.in
@@ -851,6 +851,15 @@
   DNS server.
 
 
+
+  The dns element
+  can have an optional public attribute
+  Since 1.2.17. If public
+  is "yes", then the DNS server will handle requests for all 
interfaces.
+  If public is not set or "no", the DNS server will
+  only handle requests for the interface of the virtual network.
+
+
 Currently supported sub-elements of  are:
 
   forwarder
diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng
index 4edb6eb..f70e3dc 100644
--- a/docs/schemas/network.rng
+++ b/docs/schemas/network.rng
@@ -249,6 +249,11 @@
 
   
 
+
+  
+
+  
+
 
   
 
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 72006e9..e90c004 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1332,9 +1332,14 @@ virNetworkDNSDefParseXML(const char *networkName,
 size_t i;
 int ret = -1;
 xmlNodePtr save = ctxt->node;
+char *public = NULL;
 
 ctxt->node = node;
 
+public = virXPathString("string(./@public)", ctxt);
+if (public)
+def->public = virTristateBoolTypeFromString(public);
+
 forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt);
 if (forwardPlainNames) {
 def->forwardPlainNames = 
virTristateBoolTypeFromString(forwardPlainNames);
@@ -1433,6 +1438,7 @@ virNetworkDNSDefParseXML(const char *networkName,
 
 ret = 0;
  cleanup:
+VIR_FREE(public);
 VIR_FREE(forwardPlainNames);
 VIR_FREE(fwdNodes);
 VIR_FREE(hostNodes);
@@ -2416,7 +2422,7 @@ virNetworkDNSDefFormat(virBufferPtr buf,
 size_t i, j;
 
 if (!(def->forwardPlainNames || def->nfwds || def->nhosts ||
-  def->nsrvs || def->ntxts))
+  def->nsrvs || def->ntxts || def->public))
 return 0;
 
 virBufferAddLit(buf, "nfwds || def->nhosts || def->nsrvs || def->ntxts)) {
+if (!(def->nfwds || def->nhosts || def->nsrvs || def->ntxts ||
+  def->public)) {
+virBufferAddLit(buf, "/>\n");
+return 0;
+}
+}
+
+if (def->public) {
+const char *public = virTristateBoolTypeToString(def->public);
+
+if (!public) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("Unknown public type %d in network"),
+   def->public);
+return -1;
+}
+virBufferAsprintf(buf, " public

Re: [libvirt] [Spice-devel] [PATCH v4] qemu: Use heads parameter for QXL driver

[Martin Kletzander]

On Fri, Jul 17, 2015 at 06:02:25PM +0200, Christophe Fergeau wrote:

On Fri, Jul 17, 2015 at 03:42:36PM +0200, Martin Kletzander wrote:

On Fri, Jul 17, 2015 at 12:11:55PM +0200, Peter Krempa wrote:
>On Fri, Jul 17, 2015 at 09:29:44 +0100, Frediano Ziglio wrote:
>>Allows to specify maximum number of head to QXL driver.
>>
>>Actually can be a compatiblity problem as heads in the XML configuration
>>was set by default to '1'.
>>
>>Signed-off-by: Frediano Ziglio 
>>---
>> src/qemu/qemu_capabilities.c   |  2 ++
>> src/qemu/qemu_capabilities.h   |  1 +
>> src/qemu/qemu_command.c|  5 
>> .../qemuxml2argv-video-qxl-device-max-outputs.args |  7 ++
>> .../qemuxml2argv-video-qxl-device-max-outputs.xml  | 29 
++
>> tests/qemuxml2argvtest.c   |  3 +++
>> 6 files changed, 47 insertions(+)
>> create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.args
>> create mode 100644 
tests/qemuxml2argvdata/qemuxml2argv-video-qxl-device-max-outputs.xml
>>
>>Changes from v4:
>>- rebased on new master;
>>- add test case to qemuxml2argvtest.
>
>Note review: This was added in qemu commit
>http://git.qemu.org/?p=qemu.git;a=commitdiff;h=567161fdd47aeb6987e700702f6bbfef04ae0236
>
>The commit hints that you need at least spice 0.12.6. I'll update spice
>and test this later.

I failed to update qemu without updating spice as well, so I'd say
it is safe to assume that if qemu supports it, spice will too.  And we
don't check all the lib versions for qemu runtime, that'd be a waste
of time, so this is quite alright, I think.

I updated everything spice-related and I still can't properly build
qemu particularly because of the patch you mentioned.  And I update
everything from git.  Then I looked there and I see no such function,
no wonder the build fails with undefined reference to
spice_qxl_set_monitors_config_limit.  Although that would mean that it


qemu tries to call spice_qxl_set_monitors_config_limit but the entry
point ended up being called spice_qxl_set_max_monitors in spice-server
:-/



I spend all morning fixing this to be installed properly in the
system.  Anyway, I finally managed to make this work and found out the
guest I used for it is not ready to have multiple monitors.  Anyway,
looking at everything else this definitely works, so ACK, I'll push it
in a while.

In the meantime, is the only thing this does limiting the maximum?  Is
it there just to save some memory or why?  Because otherwise I can't
see the use-case in that.  I'm not saying there isn't one, just that I
can't find it.  And I even looked under the fridge :)

Have a nice day,
Martin


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 2/2] tests: Add script to copy nodeinfo test data from host

[Andrea Bolognani]

Files we don't need, including symbolic links that might result
problematic to make dist, are removed from the copied data.
---
 tests/nodeinfodata/copy-from-host.sh | 170 +++
 1 file changed, 170 insertions(+)
 create mode 100755 tests/nodeinfodata/copy-from-host.sh

diff --git a/tests/nodeinfodata/copy-from-host.sh 
b/tests/nodeinfodata/copy-from-host.sh
new file mode 100755
index 000..b5c5e8e
--- /dev/null
+++ b/tests/nodeinfodata/copy-from-host.sh
@@ -0,0 +1,170 @@
+#!/bin/sh
+
+# copy-from-host.sh - Copy nodeinfo test data from a running host
+
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library.  If not, see
+# .
+
+SYSFS_PATH="/sys/devices/system"
+KEEP_LINKS="cpu[0-9]*"
+KEEP_DIRECTORIES="cpu
+  cpu[0-9]*
+  node
+  node[0-9]*
+  topology"
+KEEP_FILES="core_id
+core_siblings*
+kernel_max
+meminfo
+offline
+online
+physical_package_id
+possible
+present
+thread_siblings*"
+
+die() {
+typeset message=$1
+
+echo "$message" >&2
+
+exit 1
+}
+
+keep() {
+typeset tag=$1
+typeset path=$2
+
+echo "  $tag $path"
+
+return 0
+}
+
+delete() {
+typeset tag=$1
+typeset path=$2
+
+rm -rf "$path"
+
+return $?
+}
+
+is_valid_name() {
+typeset name=$1
+typeset ret=0
+
+case "$name" in
+*/*)
+# We don't want to have to deal with subdirectories, so
+# names containing slashes are rejected
+ret=1
+;;
+esac
+
+return $ret
+}
+
+matches_any_pattern() {
+typeset name=$1
+typeset patterns=$2
+typeset ret=1
+
+for pattern in $patterns; do
+case "$name" in
+$pattern)
+ret=0
+;;
+esac
+done
+
+return $ret
+}
+
+main() {
+typeset name=$1
+
+if ! test "$name"; then
+die "Usage: $SELF NAME"
+fi
+
+if ! is_valid_name "$name"; then
+die "Name '$name' is not valid"
+fi
+
+# Create directory
+if test -e "$name"; then
+die "$name: File or directory already exists"
+fi
+
+if ! mkdir "$name" >/dev/null 2>&1; then
+die "$name: Unable to create directory"
+fi
+
+echo "Copying host data..."
+
+# Copy data from host. Errors are ignored because some files we don't
+# care about always give input/output error when read
+cp -r "$SYSFS_PATH/cpu" "$name/" >/dev/null 2>&1
+cp -r "$SYSFS_PATH/node" "$name/" >/dev/null 2>&1
+
+if ! test -d "$name/cpu" || ! test -d "$name/node"; then
+die "Error while copying host data"
+fi
+
+echo "Cleaning up data..."
+
+# Delete symbolic links
+find "$name" -type l | while read l; do
+b=${l##*/}
+if matches_any_pattern "$b" "$KEEP_LINKS"; then
+keep "l" "$l"
+else
+delete "l" "$l"
+fi
+done
+
+# Delete directories
+find "$name" -type d | while read d; do
+b=${d##*/}
+# We don't want to delete the directory we just created :)
+if test "$b" = "$name"; then
+continue
+fi
+if matches_any_pattern "$b" "$KEEP_DIRECTORIES"; then
+keep "d" "$d"
+else
+delete "d" "$d"
+fi
+done
+
+# Delete files
+find "$name" -type f | while read f; do
+b=${f##*/}
+if matches_any_pattern "$b" "$KEEP_FILES"; then
+keep "f" "$f"
+else
+delete "f" "$f"
+fi
+done
+
+echo "All done"
+
+return 0
+}
+
+SELF=$0
+main "$@"
+exit $?
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 0/2] tests: Add nodeinfo test data utility scripts

[Andrea Bolognani]

Both scripts can be useful when adding new test cases to the
nodeinfo test.

Andrea Bolognani (2):
  tests: Add script to display nodeinfo test data
  tests: Add script to copy nodeinfo test data from host

 tests/nodeinfodata/copy-from-host.sh | 170 +++
 tests/nodeinfodata/display.sh| 101 +
 2 files changed, 271 insertions(+)
 create mode 100755 tests/nodeinfodata/copy-from-host.sh
 create mode 100755 tests/nodeinfodata/display.sh

-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 1/2] tests: Add script to display nodeinfo test data

[Andrea Bolognani]

---
 tests/nodeinfodata/display.sh | 101 ++
 1 file changed, 101 insertions(+)
 create mode 100755 tests/nodeinfodata/display.sh

diff --git a/tests/nodeinfodata/display.sh b/tests/nodeinfodata/display.sh
new file mode 100755
index 000..bc260e2
--- /dev/null
+++ b/tests/nodeinfodata/display.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+
+# display.sh - Display nodeinfo test data in a nice way
+
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2.1 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library.  If not, see
+# .
+
+die() {
+typeset message=$1
+
+echo "$message" >&2
+
+exit 1
+}
+
+list_cpus() {
+typeset datadir=$1
+
+ls -d "$datadir/cpu/cpu"* | while read o; do
+o=${o#*cpu/cpu}
+case "$o" in
+[0-9]*)
+echo "$o"
+;;
+esac
+done | sort -un
+}
+
+is_online() {
+typeset datadir=$1
+typeset cpu=$2
+
+typeset ret=1
+
+if test -e "$datadir/cpu/cpu$cpu/online"; then
+ret=$(cat "$datadir/cpu/cpu$cpu/online")
+fi
+
+# Reverse boolean to use it as return code
+case "$ret" in
+0) ret=1 ;;
+1) ret=0 ;;
+esac
+
+return $ret;
+}
+
+main() {
+typeset datadir=$1
+typeset threads_per_core=$2
+
+if ! test "$#" -eq "2"; then
+die "Usage: $SELF DATADIR THREADS_PER_CORE"
+fi
+if ! test -d "$datadir"; then
+die "$datadir: No such directory"
+fi
+
+echo "Threads per core: $threads_per_core"
+
+echo -n 'Present CPUs: '
+if test -e "$datadir/cpu/present"; then
+cat "$datadir/cpu/present"
+else
+echo 'information not available'
+fi
+
+for cpu in $(list_cpus "$datadir"); do
+
+mod=$(expr "$cpu" % "$threads_per_core")
+if test "$mod" = "0"; then
+echo
+fi
+
+if is_online "$datadir" "$cpu"; then
+printf " %3d*" "$cpu"
+else
+printf " %3d " "$cpu"
+fi
+done
+echo
+
+return 0
+}
+
+SELF=$0
+main "$@"
+exit $?
-- 
2.4.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] unsupported configuration: unknown video model 'virtio' - virtio-vga

[poma]

$ qemu-system-x86_64 ... -device virtio-vga


# lspci -d 1af4:1050 -knn
00:03.0 VGA compatible controller [0300]: Red Hat, Inc Device [1af4:1050] (rev 
01)
Subsystem: Red Hat, Inc Device [1af4:1100]
Kernel driver in use: virtio-pci
Kernel modules: virtio_pci


# dmesg | grep virtio
[1.727390] [drm] pci: virtio-vga detected
[1.729315] [drm] virtio vbuffers: 80 bufs, 192B each, 15kB total.
[2.023845] virtio_gpu virtio0: fb0: virtiodrmfb frame buffer device
[2.023846] virtio_gpu virtio0: registered panic notifier
[2.043135] [drm] Initialized virtio_gpu 0.0.1 0 on minor 0


# journalctl -b -u libvirtd.service -o cat 
Starting Virtualization daemon...
Started Virtualization daemon.
libvirt version: 1.2.17, package: 1.fc23 (Fedora Project, 2015-07-14-18:18:48, 
buildvm-03.phx2.fedoraproject.org)
unsupported configuration: unknown video model 'virtio'


Soonish?

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu: fix some api cannot work when disable cpuset in conf

[Luyao Huang]

https://bugzilla.redhat.com/show_bug.cgi?id=1244664

If user disable cpuset in qemu.conf, we shouldn't
try to use it, also shouldn't make some command which
can work without cpuset cannot work.

Fix these case:
1. start guest with strict numa policy (we can use libnuma help us).
2. Hot add vcpu.
3. hot add iothread.

Signed-off-by: Luyao Huang 
---
 src/qemu/qemu_cgroup.c | 16 
 src/qemu/qemu_driver.c | 10 +++---
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index 8ed74ee..640a223 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -1028,10 +1028,6 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
 if (virCgroupAddTask(cgroup_vcpu, priv->vcpupids[i]) < 0)
 goto cleanup;
 
-if (mem_mask &&
-virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0)
-goto cleanup;
-
 if (period || quota) {
 if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0)
 goto cleanup;
@@ -1041,6 +1037,10 @@ qemuSetupCgroupForVcpu(virDomainObjPtr vm)
 if (virCgroupHasController(priv->cgroup, 
VIR_CGROUP_CONTROLLER_CPUSET)) {
 virBitmapPtr cpumap = NULL;
 
+if (mem_mask &&
+virCgroupSetCpusetMems(cgroup_vcpu, mem_mask) < 0)
+goto cleanup;
+
 /* try to use the default cpu maps */
 if (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
 cpumap = priv->autoCpuset;
@@ -1205,15 +1205,15 @@ qemuSetupCgroupForIOThreads(virDomainObjPtr vm)
 goto cleanup;
 }
 
-if (mem_mask &&
-virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0)
-goto cleanup;
-
 /* Set iothreadpin in cgroup if iothreadpin xml is provided */
 if (virCgroupHasController(priv->cgroup,
VIR_CGROUP_CONTROLLER_CPUSET)) {
 virBitmapPtr cpumask = NULL;
 
+if (mem_mask &&
+virCgroupSetCpusetMems(cgroup_iothread, mem_mask) < 0)
+goto cleanup;
+
 if (def->iothreadids[i]->cpumask)
 cpumask = def->iothreadids[i]->cpumask;
 else if (def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f352a88..bb7cef4 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4597,7 +4597,9 @@ qemuDomainAddCgroupForThread(virCgroupPtr cgroup,
 if (virCgroupNewThread(cgroup, nameval, idx, true, &new_cgroup) < 0)
 return NULL;
 
-if (mem_mask && virCgroupSetCpusetMems(new_cgroup, mem_mask) < 0)
+if (mem_mask &&
+virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET) &&
+virCgroupSetCpusetMems(new_cgroup, mem_mask) < 0)
 goto error;
 
 /* Add pid/thread to the cgroup */
@@ -4653,7 +4655,8 @@ qemuDomainHotplugPinThread(virBitmapPtr cpumask,
 {
 int ret = -1;
 
-if (cgroup) {
+if (cgroup &&
+virCgroupHasController(cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
 if (qemuSetupCgroupCpusetCpus(cgroup, cpumask) < 0) {
 virReportError(VIR_ERR_OPERATION_INVALID,
_("failed to set cpuset.cpus in cgroup for id %d"),
@@ -4896,7 +4899,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int 
nvcpus,
 if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
 goto endjob;
 
-if (def && !(flags & VIR_DOMAIN_VCPU_GUEST) && virNumaIsAvailable()) {
+if (def && !(flags & VIR_DOMAIN_VCPU_GUEST) && virNumaIsAvailable() &&
+virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) {
 if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0,
false, &cgroup_temp) < 0)
 goto endjob;
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] cgroup: Drop resource partition from virSystemdMakeScopeName

[Peter Krempa]

On Fri, Jul 17, 2015 at 15:40:31 +0100, Daniel Berrange wrote:
> On Thu, Jul 16, 2015 at 04:18:08PM +0200, Peter Krempa wrote:
> > The scope name, even according to our docs is
> > "machine-$DRIVER\x2d$VMNAME.scope" virSystemdMakeScopeName would use the
> > resource partition name instead of "machine-" if it was specified thus
> > creating invalid scope paths.
> > 
> > This makes libvirt drop cgroups for a VM that uses custom resource
> > partition upon reconnecting since the detected scope name would not
> > match the expected name generated by virSystemdMakeScopeName.
> > 
> > The error is exposed by the following log entry:
> > 
> > debug : virCgroupValidateMachineGroup:302 : Name 
> > 'machine-qemu\x2dtestvm.scope' for controller 'cpu' does not match 
> > 'testvm', 'testvm.libvirt-qemu' or 'machine-test-qemu\x2dtestvm.scope'
> > 
> > for a "/machine/test" resource and "testvm" vm.
> 
> This doesn't look right - the whole idea of the resource partition
> name is that it overrides the default /machine based path. So
> ignoring the resource partition is not right, unless I'm mis-
> understanding what this patch is doing.

Perhaps I wasn't clear enough in the explanation. The code path that
calls the function where I've removed the partition is called on daemon
restart, where the paths to cgroups are re-detected upon startup.

At first virCgroupNewDetect() is called on a VM's pid and the function
detects cgroup paths for the given process.

The detected data for a VM with
/machine/asdf/test look like:
(gdb) p **group
$2 = {path = 0x7fff94000ec0 "",
  controllers = {{type = 0,
  mountPoint = 0x7fff94001330 "/sys/fs/cgroup/cpu,cpuacct", 
  linkPoint = 0x7fff94001470 "/sys/fs/cgroup/cpu",
  placement = 0x7fff94001fb0 
"/machine.slice/machine-asdf.slice/machine-asdf-test.slice/machine-qemu\\x2df20live2.scope/emulator"},
 {type = 0,
  mountPoint = 0x7fff94001200 "/sys/fs/cgroup/cpu,cpuacct",
  linkPoint = 0x7fff94001450 "/sys/fs/cgroup/cpuacct", 
  placement = 0x7fff94002020 
"/machine.slice/machine-asdf.slice/machine-asdf-test.slice/machine-qemu\\x2df20live2.scope/emulator"},
  {type = 0,
   mountPoint = 0x7fff94001410 "/sys/fs/cgroup/cpuset",
   linkPoint = 0x0,
   placement = 0x7fff94001ad0 
"/machine.slice/machine-asdf.slice/machine-asdf-test.slice/machine-qemu\\x2df20live2.scope/emulator"},

... and so on for other cgroups.

After the detection virCgroupValidateMachineGroup() is called on the
detected cgroup array to validate them with the following algorithm:

FOREACH entry IN 'cgroup' array:
Find the last directory name in entry.placement (strrchr(..., '/')).

IF the cgroup is one of CPU* cgroups AND
   the last directory component is '/emulator':

   find next previous directory component and strip '/emuator' from it


# This equals to 'machine-qemu\\x2df20live2.scope' in the example above.

# validate the name

IF directory name IS EQUAL TO 'MACHINE_NAME' OR
  'MACHINE_NAME.libvirt-DRIVER_NAME' OR
  virSystemdMakeScopeName(...):
cgroup is valid


Since the algorithm does not extract the partition name or validate it
in any way in non-systemd scenarios and in scenarios where systemd is
used the generated name wouldn't ever match since the scope name as
extracted in the code does not include the slice generated from
 the cgroups code does not work on systemd machines after
libvirt restart.

The patch as-is removes the partition name from the code that generates
the systemd SCOPE name since that does not contain it. The partition is
used to generate SLICE name which is contained in the cgroup path before
the SCOPE.

The question that comes into my mind is whether we should actually make
sure that the  corresponds to the SLICE in the detected path,
or just matching the machine SCOPE is good enough in this case.

Peter



signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qxl: Fix new function name for spice-server library

[Christophe Fergeau]

On Mon, Jul 20, 2015 at 09:43:23AM +0100, Frediano Ziglio wrote:
> The new spice-server function to limit the number of monitors (0.12.6)
> changed while development from spice_qxl_set_monitors_config_limit to
> spice_qxl_max_monitors (accepted upstream).
> By mistake I post patch with former name.
> This patch fix the function name.
> 
> Signed-off-by: Frediano Ziglio 
> 
> ---
>  hw/display/qxl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> I tested again doing a clean build, unfortunately I did some mistake
> and my tests worked.
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 4e5ff69..2288238 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -273,8 +273,7 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice 
> *qxl, int replay)
>  } else {
>  #if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
>  if (qxl->max_outputs) {
> -spice_qxl_set_monitors_config_limit(&qxl->ssd.qxl,
> -qxl->max_outputs);
> +spice_qxl_set_max_monitors(&qxl->ssd.qxl, qxl->max_outputs);
>  }
>  #endif
>  qxl->guest_monitors_config = qxl->ram->monitors_config;

ACK from me,

Christophe


pgpldifuNd0Cr.pgp
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] bhyve: add UTC clock support

[Daniel P. Berrange]

On Sun, Jul 19, 2015 at 11:20:35AM +0300, Roman Bogorodskiy wrote:
> Bhyve as of r279225 (FreeBSD -CURRENT) or r284894 (FreeBSD 10-STABLE)
> supports using UTC time offset via the '-u' argument to bhyve(8). By
> default it's still using localtime.
> 
> Make the bhyve driver use UTC clock if it's requested by specifying
>  in domain XML and if the bhyve(8) binary supports
> the '-u' flag.
> ---
>  src/bhyve/bhyve_capabilities.c | 31 
> ++
>  src/bhyve/bhyve_capabilities.h |  5 
>  src/bhyve/bhyve_command.c  | 21 +++
>  src/bhyve/bhyve_driver.c   | 13 +
>  src/bhyve/bhyve_driver.h   |  2 ++
>  src/bhyve/bhyve_utils.h|  1 +
>  .../bhyvexml2argvdata/bhyvexml2argv-acpiapic.args  |  2 +-
>  tests/bhyvexml2argvdata/bhyvexml2argv-base.args|  2 +-
>  .../bhyvexml2argv-bhyveload-explicitargs.args  |  2 +-
>  tests/bhyvexml2argvdata/bhyvexml2argv-console.args |  2 +-
>  .../bhyvexml2argv-custom-loader.args   |  2 +-
>  .../bhyvexml2argv-disk-cdrom-grub.args |  2 +-
>  .../bhyvexml2argv-disk-cdrom.args  |  2 +-
>  .../bhyvexml2argv-disk-virtio.args |  2 +-
>  .../bhyvexml2argv-grub-bootorder.args  |  2 +-
>  .../bhyvexml2argv-grub-bootorder2.args |  2 +-
>  .../bhyvexml2argv-grub-defaults.args   |  2 +-
>  .../bhyvexml2argvdata/bhyvexml2argv-localtime.args |  3 +++
>  .../bhyvexml2argv-localtime.ldargs |  1 +
>  .../bhyvexml2argvdata/bhyvexml2argv-localtime.xml  | 23 
>  tests/bhyvexml2argvdata/bhyvexml2argv-macaddr.args |  2 +-
>  .../bhyvexml2argv-serial-grub-nocons.args  |  2 +-
>  .../bhyvexml2argv-serial-grub.args |  2 +-
>  tests/bhyvexml2argvdata/bhyvexml2argv-serial.args  |  2 +-
>  tests/bhyvexml2argvtest.c  |  2 ++
>  25 files changed, 117 insertions(+), 15 deletions(-)
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-localtime.args
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-localtime.ldargs
>  create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-localtime.xml
> 
> diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c
> index 3a55879..9b21649 100644
> --- a/src/bhyve/bhyve_capabilities.c
> +++ b/src/bhyve/bhyve_capabilities.c
> @@ -141,3 +141,34 @@ virBhyveProbeGrubCaps(virBhyveGrubCapsFlags *caps)
>  VIR_FREE(binary);
>  return ret;
>  }
> +
> +int
> +virBhyveProbeCaps(virBhyveCapsFlags *caps)

This output parameter is intended to be a union of many enum
values, so you can't declare it as an enum - it should be a
plain unsigned int.

> +{
> +char *binary, *help;
> +virCommandPtr cmd = NULL;
> +int ret = 0, exit;
> +
> +binary = virFindFileInPath("bhyve");
> +if (binary == NULL)
> +goto out;
> +if (!virFileIsExecutable(binary))
> +goto out;
> +
> +cmd = virCommandNew(binary);
> +virCommandAddArg(cmd, "-h");
> +virCommandSetErrorBuffer(cmd, &help);
> +if (virCommandRun(cmd, &exit) < 0) {
> +ret = -1;
> +goto out;
> +}
> +
> +if (strstr(help, "-u:") != NULL)
> +*caps |= BHYVE_CAP_RTC_UTC;
> +
> + out:
> +VIR_FREE(help);
> +virCommandFree(cmd);
> +VIR_FREE(binary);
> +return ret;
> +}


ACK if that is fixed.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/2] docs: bhyve: document clock configuration

[Daniel P. Berrange]

On Sun, Jul 19, 2015 at 11:20:36AM +0300, Roman Bogorodskiy wrote:
> ---
>  docs/drvbhyve.html.in | 28 
>  1 file changed, 28 insertions(+)

ACK


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qxl: Fix new function name for spice-server library

[Frediano Ziglio]

The new spice-server function to limit the number of monitors (0.12.6)
changed while development from spice_qxl_set_monitors_config_limit to
spice_qxl_max_monitors (accepted upstream).
By mistake I post patch with former name.
This patch fix the function name.

Signed-off-by: Frediano Ziglio 

---
 hw/display/qxl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

I tested again doing a clean build, unfortunately I did some mistake
and my tests worked.

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 4e5ff69..2288238 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -273,8 +273,7 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice 
*qxl, int replay)
 } else {
 #if SPICE_SERVER_VERSION >= 0x000c06 /* release 0.12.6 */
 if (qxl->max_outputs) {
-spice_qxl_set_monitors_config_limit(&qxl->ssd.qxl,
-qxl->max_outputs);
+spice_qxl_set_max_monitors(&qxl->ssd.qxl, qxl->max_outputs);
 }
 #endif
 qxl->guest_monitors_config = qxl->ram->monitors_config;
-- 
2.1.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list