[libvirt] [PATCH] qemu: handle multicast overflow on macvtap for NIC_RX_FILTER

Discussion:

[libvirt] [PATCH] qemu: handle multicast overflow on macvtap for NIC_RX_FILTER_CHANGED

Jason Baron

2018-11-21 15:04:56 UTC

Guest network devices can set 'overflow' when there are a number of multicast
ips configured. For virtio_net, the limit is only 64. In this case, the list
of mac addresses is empty and the 'overflow' condition is set. Thus, the guest
will currently receive no multicast traffic in this state.

When 'overflow' is set in the guest, let's turn this into ALLMULTI on the host.

Signed-off-by: Jason Baron <***@akamai.com>
---
src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7fb9102..ea36db8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4443,11 +4443,11 @@ static void
syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
virNetDevRxFilterPtr hostFilter)
{
- if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
+ if (hostFilter->multicast.mode != guestFilter->multicast.mode ||
+ guestFilter->multicast.overflow) {
switch (guestFilter->multicast.mode) {
case VIR_NETDEV_RX_FILTER_MODE_ALL:
if (virNetDevSetRcvAllMulti(ifname, true)) {
-
VIR_WARN("Couldn't set allmulticast flag to 'on' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED", ifname);
@@ -4455,17 +4455,29 @@ syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
break;

case VIR_NETDEV_RX_FILTER_MODE_NORMAL:
- if (virNetDevSetRcvMulti(ifname, true)) {
+ if (guestFilter->multicast.overflow &&
+ (hostFilter->multicast.mode == VIR_NETDEV_RX_FILTER_MODE_ALL)) {
+ break;
+ }

+ if (virNetDevSetRcvMulti(ifname, true)) {
VIR_WARN("Couldn't set multicast flag to 'on' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED", ifname);
}

- if (virNetDevSetRcvAllMulti(ifname, false)) {
- VIR_WARN("Couldn't set allmulticast flag to 'off' for "
- "device %s while responding to "
- "NIC_RX_FILTER_CHANGED", ifname);
+ if (guestFilter->multicast.overflow == true) {
+ if (virNetDevSetRcvAllMulti(ifname, true)) {
+ VIR_WARN("Couldn't set allmulticast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+ }
+ } else {
+ if (virNetDevSetRcvAllMulti(ifname, false)) {
+ VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+ }
}
break;

--
2.7.4

Michael S. Tsirkin

2018-11-21 18:44:19 UTC

Permalink

Good catch, thanks!

Post by Jason Baron
---
src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7fb9102..ea36db8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4443,11 +4443,11 @@ static void
syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
virNetDevRxFilterPtr hostFilter)
{
- if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
+ if (hostFilter->multicast.mode != guestFilter->multicast.mode ||
+ guestFilter->multicast.overflow) {
switch (guestFilter->multicast.mode) {
if (virNetDevSetRcvAllMulti(ifname, true)) {
-
VIR_WARN("Couldn't set allmulticast flag to 'on' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED", ifname);
@@ -4455,17 +4455,29 @@ syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
break;
- if (virNetDevSetRcvMulti(ifname, true)) {
+ if (guestFilter->multicast.overflow &&
+ (hostFilter->multicast.mode == VIR_NETDEV_RX_FILTER_MODE_ALL)) {
+ break;
+ }
+ if (virNetDevSetRcvMulti(ifname, true)) {
VIR_WARN("Couldn't set multicast flag to 'on' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED", ifname);
}
- if (virNetDevSetRcvAllMulti(ifname, false)) {
- VIR_WARN("Couldn't set allmulticast flag to 'off' for "
- "device %s while responding to "
- "NIC_RX_FILTER_CHANGED", ifname);
+ if (guestFilter->multicast.overflow == true) {
+ if (virNetDevSetRcvAllMulti(ifname, true)) {
+ VIR_WARN("Couldn't set allmulticast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+ }
+ } else {
+ if (virNetDevSetRcvAllMulti(ifname, false)) {
+ VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+ }
}
break;
--
2.7.4

Michal Privoznik

2018-11-26 16:24:30 UTC

Permalink

Post by Jason Baron
Guest network devices can set 'overflow' when there are a number of multicast
ips configured. For virtio_net, the limit is only 64. In this case, the list
of mac addresses is empty and the 'overflow' condition is set. Thus, the guest
will currently receive no multicast traffic in this state.
When 'overflow' is set in the guest, let's turn this into ALLMULTI on the host.
---
src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7fb9102..ea36db8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4443,11 +4443,11 @@ static void
syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
virNetDevRxFilterPtr hostFilter)
{
- if (hostFilter->multicast.mode != guestFilter->multicast.mode) {
+ if (hostFilter->multicast.mode != guestFilter->multicast.mode ||
+ guestFilter->multicast.overflow) {
switch (guestFilter->multicast.mode) {
if (virNetDevSetRcvAllMulti(ifname, true)) {
-
VIR_WARN("Couldn't set allmulticast flag to 'on' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED", ifname);
@@ -4455,17 +4455,29 @@ syncNicRxFilterMultiMode(char *ifname, virNetDevRxFilterPtr guestFilter,
break;
- if (virNetDevSetRcvMulti(ifname, true)) {
+ if (guestFilter->multicast.overflow &&
+ (hostFilter->multicast.mode == VIR_NETDEV_RX_FILTER_MODE_ALL)) {
+ break;
+ }
+ if (virNetDevSetRcvMulti(ifname, true)) {

Ah, please do proper error check: if (func() < 0) { ... }.

Post by Jason Baron
VIR_WARN("Couldn't set multicast flag to 'on' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED", ifname);
}
- if (virNetDevSetRcvAllMulti(ifname, false)) {
- VIR_WARN("Couldn't set allmulticast flag to 'off' for "
- "device %s while responding to "
- "NIC_RX_FILTER_CHANGED", ifname);
+ if (guestFilter->multicast.overflow == true) {
+ if (virNetDevSetRcvAllMulti(ifname, true)) {
+ VIR_WARN("Couldn't set allmulticast flag to 'on' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);
+ }
+ } else {
+ if (virNetDevSetRcvAllMulti(ifname, false)) {
+ VIR_WARN("Couldn't set allmulticast flag to 'off' for "
+ "device %s while responding to "
+ "NIC_RX_FILTER_CHANGED", ifname);

I wonder if we can do something like:

if (virNetDevSetRcvAllMulti(ifname,
guestFilter->multicast.overflow) < 0) {
VIR_WARN("Couldn't set allmulticast flag to '%s' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED",
virTristateSwitchTypeToString(virTristateSwitchFromBool(guestFilter->multicast.overflow)),
ifname);
}

Otherwise looking good.

Michal

Jason Baron

2018-11-26 16:38:46 UTC

Permalink

Post by Michal Privoznik

Ah, please do proper error check: if (func() < 0) { ... }.

Ok, I didn't change this call (just moved down). Also, there are other
calls in syncNicRxFilterMultiMode() to virNetDevSetRcvMulti() and
virNetDevSetRcvAllMulti() that just check for non-zero. Should those be
changed as well? If so, I think that should done in a separate patch?

Post by Michal Privoznik

if (virNetDevSetRcvAllMulti(ifname,
guestFilter->multicast.overflow) < 0) {
VIR_WARN("Couldn't set allmulticast flag to '%s' for "
"device %s while responding to "
"NIC_RX_FILTER_CHANGED",
virTristateSwitchTypeToString(virTristateSwitchFromBool(guestFilter->multicast.overflow)),
ifname);
}

Ok, will fix.

Thanks,

-Jason