Re: [Xen-devel] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats

[Jim Fehlig]

On 12/02/2015 06:02 AM, Joao Martins wrote:
>
> On 12/02/2015 12:45 AM, Jim Fehlig wrote:
>> On 11/23/2015 11:57 AM, Joao Martins wrote:
>>> Introduce support for domainInterfaceStats API call for querying
>>> network interface statistics. Consequently it also enables the
>>> use of `virsh domifstat  ` command plus
>>> seeing the interfaces names instead of "-" when doing
>>> `virsh domiflist `.
>>>
>>> After successful guest creation we fill the network
>>> interfaces names based on domain, device id and append suffix
>>> if it's emulated in the following form: vif.[-emu].
>> One interesting Xen behavior that has existing for many, many years is that 
>> a PV
>> nic is implicitly created for each emulated nic specified in the config. The
>> guest OS picks which one to use. These days most will use the PV nic, and if
>> they are nice, "unplug" the emulated one via the unplug protocol. E.g. an HVM
>> guest with
>>
>>  
>> 
>> 
>> 
>>   
>>
>> results in two vif devices on the host
>>
>> # ip a | grep vif
>> 607: vif519.0-emu:  mtu 1500 qdisc 
>> pfifo_fast
>> master br0 state UNKNOWN group default qlen 500
>> 608: vif519.0:  mtu 1500 qdisc pfifo_fast
>> master br0 state UNKNOWN group default qlen 512
>>
>> both are connected to the bridge
>>
>> # brctl show br0
>> bridge namebridge idSTP enabled   interfaces
>> br08000.001e676598f5noeth0
>>   vif519.0
>>   vif519.0-emu
>>
>> In this case, the (not nice) guest OS is using the PV nic but did not unplug 
>> the
>> emulated one. So we have two interfaces, but the virDomainDef only contains 
>> one
>>
>> # virsh domiflist 519
>> Interface  Type   Source Model   MAC
>> ---
>> vif519.0-emu bridge br0-   00:16:3e:7a:35:ce
>>
>> Not a fault of this patch, but we'll need to figure out how to handle the
>> implicitly created PV nic. The interesting case is identifying emulated nics
>> that have been unplugged by a nice guest, and hence no longer exist in the 
>> host
>> (e.g. vif519.0-emu in the above example).
>>
> Indeed this is an issue I am aware of but wasn't sure on how to handle it. The
> way I test an HVM guest on libvirt with a (explicitly created) PV nic was with
> "model=netfront" since there wouldn't be an emulated nic.

Yep, libxl allows us specify a PV nic only, but not an emulated nic only.

>  So perhaps we could
> differ it if this was specified on HVM guests. If that wasn't the case we 
> would
> add the complementary interface along with checking if indeed the net device
> exists. On cleanup we would just delete the second interface that would appear
> with the same name.

Right. So the running config would have the implicitly added PV device. The
persistent config wouldn't change. Are you volunteering to write a patch? :-).

>
>>> We extract the network interfaces info from libxl in
>>> libxlDomainStartCallback() and make ifname . On domain
>>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
>>> being prefixed with "vif"). We also skip these two steps in case the name
>>> of the interface was manually inserted by the adminstrator.
>>>
>>> For getting the interface statistics we resort to virNetInterfaceStats
>>> and let libvirt handle the platform specific nits. Note that the latter
>>> is not yet supported in FreeBSD.
>>>
>>> Signed-off-by: Joao Martins 
>>> ---
>>> Changes since v3:
>>>  - Use libxl_device_nic_list() for getting each network interface
>>>  devid in DomainStartCallback.
>>>  - Improve error reporting by appropriately setting the right error
>>>  when no interface is known.
>>>  - Do not unlock vm if libxlDomainObjEndJob() returns false
>>>  - Set vm->def->net[i]->ifname on DomainStartCallback instead of
>>>  DomainStart.
>>>  - Change commit message reflecting the changes on the previous
>>>  item and mention correct interface names when doing domiflist.
>>>
>>> Changes since v2:
>>>  - Clear ifname if it's autogenerated, since otherwise will persist
>>>  on successive domain starts. Change commit message reflecting this
>>>  change.
>>>
>>> Changes since v1:
>>>  - Fill .ifname after domain start with generated
>>>  name from libxl  based on domain id and devid returned by libxl.
>>>  After that path validation don interfaceStats is enterily based
>>>  on ifname pretty much like the other drivers.
>>>  - Modify commit message reflecting the changes mentioned in
>>>  the previous item.
>>>  - Bump version to 1.2.22
>>> ---
>>>  src/libxl/libxl_domain.c | 29 +++
>>>  src/libxl/libxl_driver.c | 52 
>>> 
>>>  2 files changed, 81 insertions(+)
>>>
>>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>>> index 

Re: [Xen-devel] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats

[Joao Martins]

On Wed, Dec 02, 2015 at 11:50:41AM -0700, Jim Fehlig wrote:
> On 12/02/2015 06:02 AM, Joao Martins wrote:
> >
> > On 12/02/2015 12:45 AM, Jim Fehlig wrote:
> >> On 11/23/2015 11:57 AM, Joao Martins wrote:
> >>> Introduce support for domainInterfaceStats API call for querying
> >>> network interface statistics. Consequently it also enables the
> >>> use of `virsh domifstat  ` command plus
> >>> seeing the interfaces names instead of "-" when doing
> >>> `virsh domiflist `.
> >>>
> >>> After successful guest creation we fill the network
> >>> interfaces names based on domain, device id and append suffix
> >>> if it's emulated in the following form: vif.[-emu].
> >> One interesting Xen behavior that has existing for many, many years is 
> >> that a PV
> >> nic is implicitly created for each emulated nic specified in the config. 
> >> The
> >> guest OS picks which one to use. These days most will use the PV nic, and 
> >> if
> >> they are nice, "unplug" the emulated one via the unplug protocol. E.g. an 
> >> HVM
> >> guest with
> >>
> >>  
> >> 
> >> 
> >> 
> >>   
> >>
> >> results in two vif devices on the host
> >>
> >> # ip a | grep vif
> >> 607: vif519.0-emu:  mtu 1500 qdisc 
> >> pfifo_fast
> >> master br0 state UNKNOWN group default qlen 500
> >> 608: vif519.0:  mtu 1500 qdisc pfifo_fast
> >> master br0 state UNKNOWN group default qlen 512
> >>
> >> both are connected to the bridge
> >>
> >> # brctl show br0
> >> bridge namebridge idSTP enabled   interfaces
> >> br08000.001e676598f5noeth0
> >>   vif519.0
> >>   vif519.0-emu
> >>
> >> In this case, the (not nice) guest OS is using the PV nic but did not 
> >> unplug the
> >> emulated one. So we have two interfaces, but the virDomainDef only 
> >> contains one
> >>
> >> # virsh domiflist 519
> >> Interface  Type   Source Model   MAC
> >> ---
> >> vif519.0-emu bridge br0-   00:16:3e:7a:35:ce
> >>
> >> Not a fault of this patch, but we'll need to figure out how to handle the
> >> implicitly created PV nic. The interesting case is identifying emulated 
> >> nics
> >> that have been unplugged by a nice guest, and hence no longer exist in the 
> >> host
> >> (e.g. vif519.0-emu in the above example).
> >>
> > Indeed this is an issue I am aware of but wasn't sure on how to handle it. 
> > The
> > way I test an HVM guest on libvirt with a (explicitly created) PV nic was 
> > with
> > "model=netfront" since there wouldn't be an emulated nic.
> 
> Yep, libxl allows us specify a PV nic only, but not an emulated nic only.
> 
> >  So perhaps we could
> > differ it if this was specified on HVM guests. If that wasn't the case we 
> > would
> > add the complementary interface along with checking if indeed the net device
> > exists. On cleanup we would just delete the second interface that would 
> > appear
> > with the same name.
> 
> Right. So the running config would have the implicitly added PV device. The
> persistent config wouldn't change. Are you volunteering to write a patch? :-).
>
Yeap :-D

> >
> >>> We extract the network interfaces info from libxl in
> >>> libxlDomainStartCallback() and make ifname . On domain
> >>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
> >>> being prefixed with "vif"). We also skip these two steps in case the name
> >>> of the interface was manually inserted by the adminstrator.
> >>>
> >>> For getting the interface statistics we resort to virNetInterfaceStats
> >>> and let libvirt handle the platform specific nits. Note that the latter
> >>> is not yet supported in FreeBSD.
> >>>
> >>> Signed-off-by: Joao Martins 
> >>> ---
> >>> Changes since v3:
> >>>  - Use libxl_device_nic_list() for getting each network interface
> >>>  devid in DomainStartCallback.
> >>>  - Improve error reporting by appropriately setting the right error
> >>>  when no interface is known.
> >>>  - Do not unlock vm if libxlDomainObjEndJob() returns false
> >>>  - Set vm->def->net[i]->ifname on DomainStartCallback instead of
> >>>  DomainStart.
> >>>  - Change commit message reflecting the changes on the previous
> >>>  item and mention correct interface names when doing domiflist.
> >>>
> >>> Changes since v2:
> >>>  - Clear ifname if it's autogenerated, since otherwise will persist
> >>>  on successive domain starts. Change commit message reflecting this
> >>>  change.
> >>>
> >>> Changes since v1:
> >>>  - Fill .ifname after domain start with generated
> >>>  name from libxl  based on domain id and devid returned by libxl.
> >>>  After that path validation don interfaceStats is enterily based
> >>>  on ifname pretty much like the other drivers.
> >>>  - Modify commit message reflecting the changes mentioned in
> >>>  the previous 

Re: [Xen-devel] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats

[Joao Martins]

On 12/02/2015 12:45 AM, Jim Fehlig wrote:
> On 11/23/2015 11:57 AM, Joao Martins wrote:
>> Introduce support for domainInterfaceStats API call for querying
>> network interface statistics. Consequently it also enables the
>> use of `virsh domifstat  ` command plus
>> seeing the interfaces names instead of "-" when doing
>> `virsh domiflist `.
>>
>> After successful guest creation we fill the network
>> interfaces names based on domain, device id and append suffix
>> if it's emulated in the following form: vif.[-emu].
> 
> One interesting Xen behavior that has existing for many, many years is that a 
> PV
> nic is implicitly created for each emulated nic specified in the config. The
> guest OS picks which one to use. These days most will use the PV nic, and if
> they are nice, "unplug" the emulated one via the unplug protocol. E.g. an HVM
> guest with
> 
>  
> 
> 
> 
>   
> 
> results in two vif devices on the host
> 
> # ip a | grep vif
> 607: vif519.0-emu:  mtu 1500 qdisc pfifo_fast
> master br0 state UNKNOWN group default qlen 500
> 608: vif519.0:  mtu 1500 qdisc pfifo_fast
> master br0 state UNKNOWN group default qlen 512
> 
> both are connected to the bridge
> 
> # brctl show br0
> bridge namebridge idSTP enabled   interfaces
> br08000.001e676598f5noeth0
>   vif519.0
>   vif519.0-emu
> 
> In this case, the (not nice) guest OS is using the PV nic but did not unplug 
> the
> emulated one. So we have two interfaces, but the virDomainDef only contains 
> one
> 
> # virsh domiflist 519
> Interface  Type   Source Model   MAC
> ---
> vif519.0-emu bridge br0-   00:16:3e:7a:35:ce
> 
> Not a fault of this patch, but we'll need to figure out how to handle the
> implicitly created PV nic. The interesting case is identifying emulated nics
> that have been unplugged by a nice guest, and hence no longer exist in the 
> host
> (e.g. vif519.0-emu in the above example).
> 
Indeed this is an issue I am aware of but wasn't sure on how to handle it. The
way I test an HVM guest on libvirt with a (explicitly created) PV nic was with
"model=netfront" since there wouldn't be an emulated nic. So perhaps we could
differ it if this was specified on HVM guests. If that wasn't the case we would
add the complementary interface along with checking if indeed the net device
exists. On cleanup we would just delete the second interface that would appear
with the same name.

>> We extract the network interfaces info from libxl in
>> libxlDomainStartCallback() and make ifname . On domain
>> cleanup we also clear ifname, in case it was set by libvirt (i.e.
>> being prefixed with "vif"). We also skip these two steps in case the name
>> of the interface was manually inserted by the adminstrator.
>>
>> For getting the interface statistics we resort to virNetInterfaceStats
>> and let libvirt handle the platform specific nits. Note that the latter
>> is not yet supported in FreeBSD.
>>
>> Signed-off-by: Joao Martins 
>> ---
>> Changes since v3:
>>  - Use libxl_device_nic_list() for getting each network interface
>>  devid in DomainStartCallback.
>>  - Improve error reporting by appropriately setting the right error
>>  when no interface is known.
>>  - Do not unlock vm if libxlDomainObjEndJob() returns false
>>  - Set vm->def->net[i]->ifname on DomainStartCallback instead of
>>  DomainStart.
>>  - Change commit message reflecting the changes on the previous
>>  item and mention correct interface names when doing domiflist.
>>
>> Changes since v2:
>>  - Clear ifname if it's autogenerated, since otherwise will persist
>>  on successive domain starts. Change commit message reflecting this
>>  change.
>>
>> Changes since v1:
>>  - Fill .ifname after domain start with generated
>>  name from libxl  based on domain id and devid returned by libxl.
>>  After that path validation don interfaceStats is enterily based
>>  on ifname pretty much like the other drivers.
>>  - Modify commit message reflecting the changes mentioned in
>>  the previous item.
>>  - Bump version to 1.2.22
>> ---
>>  src/libxl/libxl_domain.c | 29 +++
>>  src/libxl/libxl_driver.c | 52 
>> 
>>  2 files changed, 81 insertions(+)
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index a7267b0..141f241 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>  }
>>  }
>>  
>> +if ((vm->def->nnets)) {
>> +ssize_t i;
> 
> size_t
> 
Apologies for the typo.

>> +
>> +for (i = 0; i < vm->def->nnets; i++) {
>> +virDomainNetDefPtr net = vm->def->nets[i];

Re: [Xen-devel] [PATCH v2 2/2] libxl: implement virDomainInterfaceStats

[Jim Fehlig]

On 11/23/2015 11:57 AM, Joao Martins wrote:
> Introduce support for domainInterfaceStats API call for querying
> network interface statistics. Consequently it also enables the
> use of `virsh domifstat  ` command plus
> seeing the interfaces names instead of "-" when doing
> `virsh domiflist `.
>
> After successful guest creation we fill the network
> interfaces names based on domain, device id and append suffix
> if it's emulated in the following form: vif.[-emu].

One interesting Xen behavior that has existing for many, many years is that a PV
nic is implicitly created for each emulated nic specified in the config. The
guest OS picks which one to use. These days most will use the PV nic, and if
they are nice, "unplug" the emulated one via the unplug protocol. E.g. an HVM
guest with

 



  

results in two vif devices on the host

# ip a | grep vif
607: vif519.0-emu:  mtu 1500 qdisc pfifo_fast
master br0 state UNKNOWN group default qlen 500
608: vif519.0:  mtu 1500 qdisc pfifo_fast
master br0 state UNKNOWN group default qlen 512

both are connected to the bridge

# brctl show br0
bridge namebridge idSTP enabled   interfaces
br08000.001e676598f5noeth0
  vif519.0
  vif519.0-emu

In this case, the (not nice) guest OS is using the PV nic but did not unplug the
emulated one. So we have two interfaces, but the virDomainDef only contains one

# virsh domiflist 519
Interface  Type   Source Model   MAC
---
vif519.0-emu bridge br0-   00:16:3e:7a:35:ce

Not a fault of this patch, but we'll need to figure out how to handle the
implicitly created PV nic. The interesting case is identifying emulated nics
that have been unplugged by a nice guest, and hence no longer exist in the host
(e.g. vif519.0-emu in the above example).

> We extract the network interfaces info from libxl in
> libxlDomainStartCallback() and make ifname . On domain
> cleanup we also clear ifname, in case it was set by libvirt (i.e.
> being prefixed with "vif"). We also skip these two steps in case the name
> of the interface was manually inserted by the adminstrator.
>
> For getting the interface statistics we resort to virNetInterfaceStats
> and let libvirt handle the platform specific nits. Note that the latter
> is not yet supported in FreeBSD.
>
> Signed-off-by: Joao Martins 
> ---
> Changes since v3:
>  - Use libxl_device_nic_list() for getting each network interface
>  devid in DomainStartCallback.
>  - Improve error reporting by appropriately setting the right error
>  when no interface is known.
>  - Do not unlock vm if libxlDomainObjEndJob() returns false
>  - Set vm->def->net[i]->ifname on DomainStartCallback instead of
>  DomainStart.
>  - Change commit message reflecting the changes on the previous
>  item and mention correct interface names when doing domiflist.
>
> Changes since v2:
>  - Clear ifname if it's autogenerated, since otherwise will persist
>  on successive domain starts. Change commit message reflecting this
>  change.
>
> Changes since v1:
>  - Fill .ifname after domain start with generated
>  name from libxl  based on domain id and devid returned by libxl.
>  After that path validation don interfaceStats is enterily based
>  on ifname pretty much like the other drivers.
>  - Modify commit message reflecting the changes mentioned in
>  the previous item.
>  - Bump version to 1.2.22
> ---
>  src/libxl/libxl_domain.c | 29 +++
>  src/libxl/libxl_driver.c | 52 
> 
>  2 files changed, 81 insertions(+)
>
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index a7267b0..141f241 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -728,6 +728,17 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>  }
>  }
>  
> +if ((vm->def->nnets)) {
> +ssize_t i;

size_t

> +
> +for (i = 0; i < vm->def->nnets; i++) {
> +virDomainNetDefPtr net = vm->def->nets[i];
> +
> +if (STRPREFIX(net->ifname, "vif"))
> +VIR_FREE(net->ifname);
> +}
> +}
> +
>  if (virAsprintf(, "%s/%s.xml", cfg->stateDir, vm->def->name) > 0) {
>  if (unlink(file) < 0 && errno != ENOENT && errno != ENOTDIR)
>  VIR_DEBUG("Failed to remove domain XML for %s", vm->def->name);
> @@ -857,6 +868,8 @@ static void
>  libxlDomainStartCallback(libxl_ctx *ctx, libxl_event *ev, void *for_callback)
>  {
>  virDomainObjPtr vm = for_callback;
> +libxl_device_nic *nics;
> +int nnics;
>  size_t i;
>  
>  virObjectLock(vm);
> @@ -883,6 +896,22 @@ libxlDomainStartCallback(libxl_ctx *ctx, libxl_event 
> *ev, void *for_callback)
>