Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Amos Kong ak...@redhat.com writes: On Thu, Jul 04, 2013 at 08:28:59AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: [...] This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? We will not. It's ugly to check if nic is virtio-net nic in net/net.c, so I register the query function to NetClientInfo. Traversal the net client list in net/net.c, and execute query of each virtio-net instance in virtio-net.c Implementing the feature as an optional callback is fine. Let me rephrase my question: could this feature be implemented for other NICs? I'm *not* asking you to do that, just whether it would be possible. I'm asking because my review of the QAPI schema depends on the answer. 2. If the former, are you reasonably sure this object will do for other NICs? No. I'm not sure I understand you. Do you mean to say that the feature could be implemented for other NICs, but RxFilterInfo would probably not fit for them? We will not implement the feature to other NICs, no request. We notify the management of virtio-net rx-filter change, because we want to sync the the rx-filter change to macvtap device. I understand there are no plans to implement this feature for other NICs. But I'm not asking whether we *want* to implement it for other NICs, I'm asking whether we *could*. In theory, we can. Or rephrased yet another way: what exactly makes this feature applicable to virtio-net only? Macvtap can only be used by virtio-net, not other emulated nic. It's meaningless for management to know the rx-filter change of non-virtio-net NICs. I'm having trouble squaring in theory, we can with meaningless. So I'm rephrasing my question yet again. Do NICs other than virtio-net have rx-filters? Yes. Talked with Jason, upstream kernel fixed some bugs, then we can also use e1000 with macvtap. In this case, we should also implement a .query_rx_filter function for e1000. We can do it by another patchset. Yes. Just to avoid misunderstandings: I'm not asking you for that. I merely asked whether it's possible, and you answered that. If yes, what have these NIC rx-filters in common, and how do they differ? Why would anybody want to query rx-filters? Use cases, please. It's a way to tell management the rx-filter setup in guest nic. Management query the rx-filter setup of guest, then change the setup of macvtap device, macvtap uses same rx-filter setup as virtual nic. So this use case is mirror the virtual NIC's rx-filter setup to macvtap. Makes sense. This leads me to the question I've been aiming for all along: will your definition of RxFilterInfo do for devices other than virtio-net? It should do if it contains only stuff that all NICs with an rx-filter have. Is that the case? [...]
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, Jul 12, 2013 at 08:32:29AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Thu, Jul 04, 2013 at 08:28:59AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: [...] This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? We will not. It's ugly to check if nic is virtio-net nic in net/net.c, so I register the query function to NetClientInfo. Traversal the net client list in net/net.c, and execute query of each virtio-net instance in virtio-net.c Implementing the feature as an optional callback is fine. Let me rephrase my question: could this feature be implemented for other NICs? I'm *not* asking you to do that, just whether it would be possible. I'm asking because my review of the QAPI schema depends on the answer. 2. If the former, are you reasonably sure this object will do for other NICs? No. I'm not sure I understand you. Do you mean to say that the feature could be implemented for other NICs, but RxFilterInfo would probably not fit for them? We will not implement the feature to other NICs, no request. We notify the management of virtio-net rx-filter change, because we want to sync the the rx-filter change to macvtap device. I understand there are no plans to implement this feature for other NICs. But I'm not asking whether we *want* to implement it for other NICs, I'm asking whether we *could*. In theory, we can. Or rephrased yet another way: what exactly makes this feature applicable to virtio-net only? Macvtap can only be used by virtio-net, not other emulated nic. It's meaningless for management to know the rx-filter change of non-virtio-net NICs. I'm having trouble squaring in theory, we can with meaningless. So I'm rephrasing my question yet again. Do NICs other than virtio-net have rx-filters? Yes. Talked with Jason, upstream kernel fixed some bugs, then we can also use e1000 with macvtap. In this case, we should also implement a .query_rx_filter function for e1000. We can do it by another patchset. Yes. Just to avoid misunderstandings: I'm not asking you for that. I merely asked whether it's possible, and you answered that. If yes, what have these NIC rx-filters in common, and how do they differ? Why would anybody want to query rx-filters? Use cases, please. It's a way to tell management the rx-filter setup in guest nic. Management query the rx-filter setup of guest, then change the setup of macvtap device, macvtap uses same rx-filter setup as virtual nic. So this use case is mirror the virtual NIC's rx-filter setup to macvtap. Makes sense. This leads me to the question I've been aiming for all along: will your definition of RxFilterInfo do for devices other than virtio-net? query_rx_filter() returns the address of nic's RxFilterInfo. RxFilterInfo is general. It should do if it contains only stuff that all NICs with an rx-filter have. Is that the case? Yes. -- Amos.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, Jul 04, 2013 at 08:28:59AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: [...] This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? We will not. It's ugly to check if nic is virtio-net nic in net/net.c, so I register the query function to NetClientInfo. Traversal the net client list in net/net.c, and execute query of each virtio-net instance in virtio-net.c Implementing the feature as an optional callback is fine. Let me rephrase my question: could this feature be implemented for other NICs? I'm *not* asking you to do that, just whether it would be possible. I'm asking because my review of the QAPI schema depends on the answer. 2. If the former, are you reasonably sure this object will do for other NICs? No. I'm not sure I understand you. Do you mean to say that the feature could be implemented for other NICs, but RxFilterInfo would probably not fit for them? We will not implement the feature to other NICs, no request. We notify the management of virtio-net rx-filter change, because we want to sync the the rx-filter change to macvtap device. I understand there are no plans to implement this feature for other NICs. But I'm not asking whether we *want* to implement it for other NICs, I'm asking whether we *could*. In theory, we can. Or rephrased yet another way: what exactly makes this feature applicable to virtio-net only? Macvtap can only be used by virtio-net, not other emulated nic. It's meaningless for management to know the rx-filter change of non-virtio-net NICs. I'm having trouble squaring in theory, we can with meaningless. So I'm rephrasing my question yet again. Do NICs other than virtio-net have rx-filters? Yes. Talked with Jason, upstream kernel fixed some bugs, then we can also use e1000 with macvtap. In this case, we should also implement a .query_rx_filter function for e1000. We can do it by another patchset. If yes, what have these NIC rx-filters in common, and how do they differ? Why would anybody want to query rx-filters? Use cases, please. It's a way to tell management the rx-filter setup in guest nic. Management query the rx-filter setup of guest, then change the setup of macvtap device, macvtap uses same rx-filter setup as virtual nic. Why is querying rx-filters meaningless for anything but virtio-net? The dictionary explains meaningless as having no meaning; of no value. Thus, for the query to be meaningless, the answer must carry no information, or at least none of value. Is querying rx-filters really meaningless? Or is it just something we don't need right now, and can't see being needed in the future? -- Amos.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: [...] This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? We will not. It's ugly to check if nic is virtio-net nic in net/net.c, so I register the query function to NetClientInfo. Traversal the net client list in net/net.c, and execute query of each virtio-net instance in virtio-net.c Implementing the feature as an optional callback is fine. Let me rephrase my question: could this feature be implemented for other NICs? I'm *not* asking you to do that, just whether it would be possible. I'm asking because my review of the QAPI schema depends on the answer. 2. If the former, are you reasonably sure this object will do for other NICs? No. I'm not sure I understand you. Do you mean to say that the feature could be implemented for other NICs, but RxFilterInfo would probably not fit for them? We will not implement the feature to other NICs, no request. We notify the management of virtio-net rx-filter change, because we want to sync the the rx-filter change to macvtap device. I understand there are no plans to implement this feature for other NICs. But I'm not asking whether we *want* to implement it for other NICs, I'm asking whether we *could*. In theory, we can. Or rephrased yet another way: what exactly makes this feature applicable to virtio-net only? Macvtap can only be used by virtio-net, not other emulated nic. It's meaningless for management to know the rx-filter change of non-virtio-net NICs. I'm having trouble squaring in theory, we can with meaningless. So I'm rephrasing my question yet again. Do NICs other than virtio-net have rx-filters? If yes, what have these NIC rx-filters in common, and how do they differ? Why would anybody want to query rx-filters? Use cases, please. Why is querying rx-filters meaningless for anything but virtio-net? The dictionary explains meaningless as having no meaning; of no value. Thus, for the query to be meaningless, the answer must carry no information, or at least none of value. Is querying rx-filters really meaningless? Or is it just something we don't need right now, and can't see being needed in the future? If the answer is nothing, then we *could* implement it for other NICs. Else, implementing it for other NICs would be impossible. Once again, I'm not asking because I want it implemented for other NICs. I'm asking because the answer affects my review of the schema.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Tue, Jul 02, 2013 at 03:27:12PM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: [...] This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? We will not. It's ugly to check if nic is virtio-net nic in net/net.c, so I register the query function to NetClientInfo. Traversal the net client list in net/net.c, and execute query of each virtio-net instance in virtio-net.c Implementing the feature as an optional callback is fine. Let me rephrase my question: could this feature be implemented for other NICs? I'm *not* asking you to do that, just whether it would be possible. I'm asking because my review of the QAPI schema depends on the answer. 2. If the former, are you reasonably sure this object will do for other NICs? No. I'm not sure I understand you. Do you mean to say that the feature could be implemented for other NICs, but RxFilterInfo would probably not fit for them? We will not implement the feature to other NICs, no request. We notify the management of virtio-net rx-filter change, because we want to sync the the rx-filter change to macvtap device. I understand there are no plans to implement this feature for other NICs. But I'm not asking whether we *want* to implement it for other NICs, I'm asking whether we *could*. In theory, we can. Or rephrased yet another way: what exactly makes this feature applicable to virtio-net only? Macvtap can only be used by virtio-net, not other emulated nic. It's meaningless for management to know the rx-filter change of non-virtio-net NICs. If the answer is nothing, then we *could* implement it for other NICs. Else, implementing it for other NICs would be impossible. Once again, I'm not asking because I want it implemented for other NICs. I'm asking because the answer affects my review of the schema. -- Amos.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Wed, Jun 26, 2013 at 11:54:20AM +0200, Markus Armbruster wrote:
Amos Kong ak...@redhat.com writes:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
Thanks for your comments, some comments are out-of-data, I removed
them in this reply.
+static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
+{
+VirtIONet *n = qemu_get_nic_opaque(nc);
+RxFilterInfo *info;
+strList *str_list = NULL;
+strList *entry;
+int i;
+
+info = g_malloc0(sizeof(*info));
+info-name = g_strdup(nc-name);
+info-promiscuous = n-promisc;
+
+if (n-nouni) {
+info-unicast = RX_STATE_NO;
+} else if (n-alluni) {
+info-unicast = RX_STATE_ALL;
+} else {
+info-unicast = RX_STATE_NORMAL;
+}
+
+if (n-nomulti) {
+info-multicast = RX_STATE_NO;
+} else if (n-allmulti) {
+info-multicast = RX_STATE_ALL;
+} else {
+info-multicast = RX_STATE_NORMAL;
+}
Makes me wonder whether replacing VirtIONet members noFOO and allFOO by
an enum RxState would make things clearer there. Outside the scope of
this patch.
Good suggestion, added to my todolist.
+
+info-broadcast_allowed = n-nobcast;
+info-multicast_overflow = n-mac_table.multi_overflow;
+info-unicast_overflow = n-mac_table.uni_overflow;
+info-main_mac = g_strdup_printf(%.2x:%.2x:%.2x:%.2x:%.2x:%.2x,
+ n-mac[0], n-mac[1], n-mac[2],
+ n-mac[3], n-mac[4], n-mac[5]);
+
Please initialize str_list here rather than at its declaration, because
that'll make the loop's workings more locally obvious, and because...
+for (i = 0; i n-mac_table.first_multi; i++) {
+entry = g_malloc0(sizeof(*entry));
+entry-value = g_strdup_printf(%.2x:%.2x:%.2x:%.2x:%.2x:%.2x,
+ n-mac_table.macs[i * ETH_ALEN],
+ n-mac_table.macs[i * ETH_ALEN + 1],
+ n-mac_table.macs[i * ETH_ALEN + 2],
+ n-mac_table.macs[i * ETH_ALEN + 3],
+ n-mac_table.macs[i * ETH_ALEN + 4],
+ n-mac_table.macs[i * ETH_ALEN +
5]);
+entry-next = str_list;
+str_list = entry;
+}
+info-unicast_table = str_list;
+
... it's how this loop works.
Actually, the two loops are duplicates. Consider factoring out a helper
function.
+return info;
+}
+
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
@@ -442,6 +528,8 @@ static int virtio_net_handle_rx_mode(VirtIONet *n,
uint8_t cmd,
return VIRTIO_NET_ERR;
}
+rxfilter_notify(n-netclient_name);
+
return VIRTIO_NET_OK;
}
@@ -495,6 +583,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
cmd,
s = iov_to_buf(iov, iov_cnt, 0, n-mac, sizeof(n-mac));
assert(s == sizeof(n-mac));
qemu_format_nic_info_str(qemu_get_queue(n-nic), n-mac);
+rxfilter_notify(n-netclient_name);
+
return VIRTIO_NET_OK;
}
@@ -559,6 +649,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
cmd,
n-mac_table.multi_overflow = 1;
}
+rxfilter_notify(n-netclient_name);
+
return VIRTIO_NET_OK;
}
The error returns don't trigger the event. We can fail after clearing
n-mac_table. Why is that okay?
We should notify in error path if n-mac_table is changed.
@@ -1312,6 +1404,7 @@ static NetClientInfo net_virtio_info = {
.receive = virtio_net_receive,
.cleanup = virtio_net_cleanup,
.link_status_changed = virtio_net_set_link_status,
+.query_rx_filter = virtio_net_query_rxfilter,
};
static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
diff --git a/include/net/net.h b/include/net/net.h
index 43d85a1..0af5ba3 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -49,6 +49,7 @@ typedef ssize_t
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Amos Kong ak...@redhat.com writes:
On Wed, Jun 26, 2013 at 11:54:20AM +0200, Markus Armbruster wrote:
Amos Kong ak...@redhat.com writes:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
Thanks for your comments, some comments are out-of-data, I removed
them in this reply.
Okay :)
+static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
+{
+VirtIONet *n = qemu_get_nic_opaque(nc);
+RxFilterInfo *info;
+strList *str_list = NULL;
+strList *entry;
+int i;
+
+info = g_malloc0(sizeof(*info));
+info-name = g_strdup(nc-name);
+info-promiscuous = n-promisc;
+
+if (n-nouni) {
+info-unicast = RX_STATE_NO;
+} else if (n-alluni) {
+info-unicast = RX_STATE_ALL;
+} else {
+info-unicast = RX_STATE_NORMAL;
+}
+
+if (n-nomulti) {
+info-multicast = RX_STATE_NO;
+} else if (n-allmulti) {
+info-multicast = RX_STATE_ALL;
+} else {
+info-multicast = RX_STATE_NORMAL;
+}
Makes me wonder whether replacing VirtIONet members noFOO and allFOO by
an enum RxState would make things clearer there. Outside the scope of
this patch.
Good suggestion, added to my todolist.
+
+info-broadcast_allowed = n-nobcast;
+info-multicast_overflow = n-mac_table.multi_overflow;
+info-unicast_overflow = n-mac_table.uni_overflow;
+info-main_mac = g_strdup_printf(%.2x:%.2x:%.2x:%.2x:%.2x:%.2x,
+ n-mac[0], n-mac[1], n-mac[2],
+ n-mac[3], n-mac[4], n-mac[5]);
+
Please initialize str_list here rather than at its declaration, because
that'll make the loop's workings more locally obvious, and because...
+for (i = 0; i n-mac_table.first_multi; i++) {
+entry = g_malloc0(sizeof(*entry));
+entry-value = g_strdup_printf(%.2x:%.2x:%.2x:%.2x:%.2x:%.2x,
+ n-mac_table.macs[i * ETH_ALEN],
+ n-mac_table.macs[i * ETH_ALEN +
1],
+ n-mac_table.macs[i * ETH_ALEN +
2],
+ n-mac_table.macs[i * ETH_ALEN +
3],
+ n-mac_table.macs[i * ETH_ALEN +
4],
+ n-mac_table.macs[i * ETH_ALEN + 5]);
+entry-next = str_list;
+str_list = entry;
+}
+info-unicast_table = str_list;
+
... it's how this loop works.
Actually, the two loops are duplicates. Consider factoring out a helper
function.
+return info;
+}
+
static void virtio_net_reset(VirtIODevice *vdev)
{
VirtIONet *n = VIRTIO_NET(vdev);
@@ -442,6 +528,8 @@ static int virtio_net_handle_rx_mode(VirtIONet *n,
uint8_t cmd,
return VIRTIO_NET_ERR;
}
+rxfilter_notify(n-netclient_name);
+
return VIRTIO_NET_OK;
}
@@ -495,6 +583,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
cmd,
s = iov_to_buf(iov, iov_cnt, 0, n-mac, sizeof(n-mac));
assert(s == sizeof(n-mac));
qemu_format_nic_info_str(qemu_get_queue(n-nic), n-mac);
+rxfilter_notify(n-netclient_name);
+
return VIRTIO_NET_OK;
}
@@ -559,6 +649,8 @@ static int virtio_net_handle_mac(VirtIONet *n, uint8_t
cmd,
n-mac_table.multi_overflow = 1;
}
+rxfilter_notify(n-netclient_name);
+
return VIRTIO_NET_OK;
}
The error returns don't trigger the event. We can fail after clearing
n-mac_table. Why is that okay?
We should notify in error path if n-mac_table is changed.
@@ -1312,6 +1404,7 @@ static NetClientInfo net_virtio_info = {
.receive = virtio_net_receive,
.cleanup = virtio_net_cleanup,
.link_status_changed = virtio_net_set_link_status,
+.query_rx_filter = virtio_net_query_rxfilter,
};
static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
diff --git a/include/net/net.h b/include/net/net.h
index 43d85a1..0af5ba3 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -49,6 +49,7
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote:
Amos Kong ak...@redhat.com writes:
diff --git a/net/net.c b/net/net.c
index 43a74e4..7b73a10 100644
--- a/net/net.c
+++ b/net/net.c
@@ -961,6 +961,53 @@ void print_net_client(Monitor *mon, NetClientState
*nc)
nc-info_str);
}
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
+continue;
+}
+
+if (nc-info-query_rx_filter) {
+info = nc-info-query_rx_filter(nc);
+entry = g_malloc0(sizeof(*entry));
+entry-value = info;
+
+if (!filter_list) {
+filter_list = entry;
+} else {
+last_entry-next = entry;
+}
+last_entry = entry;
+} else if (has_name) {
+error_setg(errp, net client(%s) doesn't support
+rx-filter querying, name);
+break;
+}
+
+}
+
+if (filter_list == NULL !error_is_set(errp)) {
+if (has_name) {
+error_setg(errp, invalid net client name: %s, name);
+} else {
+error_setg(errp, no net client supports rx-filter
querying);
Why is this an error?
Return an error note is bettr than a NULL list. Management should not query
the rx-filter info if there is no net client supports it.
I disagree. If a management application asks a question we can answer,
we should give it a straight answer, not an error.
ls of an empty directory prints nothing and succeeds. It doesn't
concern itself with whether I should or should not ask for the
directory's contents, say because nobody but me can access the
directory, and I therefore should know perfectly well that it's empty.
Reasonable.
{
return: [
]
}
NULL list might be misread to that it supports rx-filter querying, but the
rx-filter config has no item.
If such a misunderstanding is possible, the command's specification
needs fixing.
If I read the specification correctly, the returned list has one element
per selected NIC that supports rx-filter querying, and no more.
Without the optional argument, all NICs are selected. With the optional
argument, just the NIC identified by the argument is selected.
We don't support filter by net client name in latest version.
Whether the rx-filter config has no item shouldn't matter. I don't
even understand what that means :)
I'm wrong, if one nic doesn't have rx-filter config entry, a NULL dict
will also be returned.
return: [
{ }
]
I will return NULL table in this case.
+}
+}
+
+return filter_list;
+}
+
+Example:
+
+- { execute: query-rx-filter, arguments: { name: vnet0 }}
+- { return: [
+{
+promiscuous: true,
+name: vnet0,
+main-mac: 52:54:00:12:34:56,
+unicast: normal,
+unicast-table: [
+],
+multicast: normal,
+multicast-overflow: false,
+unicast-overflow: false,
+multicast-table: [
+01:00:5e:00:00:01,
+33:33:00:00:00:01,
+33:33:ff:12:34:56
+],
+broadcast-allowed: false
+}
+ ]
+ }
+
+EQMP
This interface is abstract in the sense that it applies to all NICs. At
this time, it's implemented only virtio-net implements it. I'm
habitually wary of abstractions based on just one concrete instance,
which makes me ask:
1. Ignorant question first: could the feature make sense for other NICs,
too, or is it specific to virtio-net?
We will not.
It's ugly to check if nic is virtio-net nic in net/net.c, so I
register the query function to NetClientInfo. Traversal the net
client list in net/net.c, and execute query of each virtio-net
instance in virtio-net.c
Implementing the feature as an optional callback is fine.
Let me rephrase my question: could this feature be implemented for other
NICs? I'm *not* asking you to do that, just whether it would be
possible.
I'm asking because my review of the QAPI schema depends on the answer.
2. If the former, are you reasonably sure this object will do for other
NICs?
No.
I'm not sure I understand you. Do you mean to say that the
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Amos Kong ak...@redhat.com writes: On Tue, Jul 02, 2013 at 11:05:56AM +0200, Markus Armbruster wrote: Amos Kong ak...@redhat.com writes: [...] This interface is abstract in the sense that it applies to all NICs. At this time, it's implemented only virtio-net implements it. I'm habitually wary of abstractions based on just one concrete instance, which makes me ask: 1. Ignorant question first: could the feature make sense for other NICs, too, or is it specific to virtio-net? We will not. It's ugly to check if nic is virtio-net nic in net/net.c, so I register the query function to NetClientInfo. Traversal the net client list in net/net.c, and execute query of each virtio-net instance in virtio-net.c Implementing the feature as an optional callback is fine. Let me rephrase my question: could this feature be implemented for other NICs? I'm *not* asking you to do that, just whether it would be possible. I'm asking because my review of the QAPI schema depends on the answer. 2. If the former, are you reasonably sure this object will do for other NICs? No. I'm not sure I understand you. Do you mean to say that the feature could be implemented for other NICs, but RxFilterInfo would probably not fit for them? We will not implement the feature to other NICs, no request. We notify the management of virtio-net rx-filter change, because we want to sync the the rx-filter change to macvtap device. I understand there are no plans to implement this feature for other NICs. But I'm not asking whether we *want* to implement it for other NICs, I'm asking whether we *could*. Or rephrased yet another way: what exactly makes this feature applicable to virtio-net only? If the answer is nothing, then we *could* implement it for other NICs. Else, implementing it for other NICs would be impossible. Once again, I'm not asking because I want it implemented for other NICs. I'm asking because the answer affects my review of the schema.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, Jun 28, 2013 at 07:27:21PM +0200, Markus Armbruster wrote: Eric Blake ebl...@redhat.com writes: On 06/26/2013 08:15 AM, Markus Armbruster wrote: Luiz Capitulino lcapitul...@redhat.com writes: On Wed, 26 Jun 2013 14:00:30 +0200 Markus Armbruster arm...@redhat.com wrote: Meh, I got confused and reviewed an out-of-date version. Hope it's not entirely noise. I don't think it is. But this series got applied to Michael's tree, so if you want your comments addressed before applying to master (I think we do) then it's better to state it clearly. Michael, please give Amos a chance to reply to my review. Looks like the pull request is already live but had issues; meanwhile, I also found an issue when reviewing the pull request: https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg05127.html Missed it, thanks! In mst's v2 pull, it still contains my patch, it's not merged to master by Anthony. I think mst should re-send a v3 without my patch as said in changelog. I will fix the problem in your reply, thanks. -- Amos.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On 06/26/2013 08:15 AM, Markus Armbruster wrote: Luiz Capitulino lcapitul...@redhat.com writes: On Wed, 26 Jun 2013 14:00:30 +0200 Markus Armbruster arm...@redhat.com wrote: Meh, I got confused and reviewed an out-of-date version. Hope it's not entirely noise. I don't think it is. But this series got applied to Michael's tree, so if you want your comments addressed before applying to master (I think we do) then it's better to state it clearly. Michael, please give Amos a chance to reply to my review. Looks like the pull request is already live but had issues; meanwhile, I also found an issue when reviewing the pull request: https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg05127.html -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Eric Blake ebl...@redhat.com writes: On 06/26/2013 08:15 AM, Markus Armbruster wrote: Luiz Capitulino lcapitul...@redhat.com writes: On Wed, 26 Jun 2013 14:00:30 +0200 Markus Armbruster arm...@redhat.com wrote: Meh, I got confused and reviewed an out-of-date version. Hope it's not entirely noise. I don't think it is. But this series got applied to Michael's tree, so if you want your comments addressed before applying to master (I think we do) then it's better to state it clearly. Michael, please give Amos a chance to reply to my review. Looks like the pull request is already live but had issues; meanwhile, I also found an issue when reviewing the pull request: https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg05127.html Missed it, thanks!
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Eric Blake ebl...@redhat.com writes:
On 05/23/2013 03:08 AM, Amos Kong wrote:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
s/don't/doesn't/
rx-filter after receives one event, new events won't be sent
s/after/after it/
to qmp monitor.
+++ b/net/net.c
@@ -961,6 +961,53 @@ void print_net_client(Monitor *mon, NetClientState *nc)
nc-info_str);
}
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
Do you need the has_name argument here, or can you ensure that the
caller passes NULL when the caller's has_name was false, for one less
parameter and the same amount of information?
QAPI always generates a has_FOO parameter for an optional FOO parameter,
even when FOO is a pointer, which has the perfectly obvious special not
given value NULL. I hate that.
[...]
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Eric Blake ebl...@redhat.com writes: On 05/24/2013 07:08 AM, Luiz Capitulino wrote: This sounds like premature optimization to me, but I wonder if instead of cluttering commands with arguments to do the filtering we could add some standard way of doing this in the QAPI. Maybe we could make QAPI support generic filtering for all query-* commands. But there's still a cost with making all query-* commands malloc the space for an entire list only to then have a QAPI layer on top of it do filtering before handing back the single element over the wire, compared to passing the filtering down to the query-* implementation to do the filtering in place. I'd expect the time spent on malloc to be dwarved several times over by I/O latency. If we worry about malloc impacting our latency, then we should not be using QAPI! It's really, really malloc-happy. In QMP, we generally don't. It was you who suggested a filter command? No, Stefan suggested it on v1: https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03102.html So far, I'm with Luiz here.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Amos Kong ak...@redhat.com writes:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
---
hmp-commands.hx | 2 ++
hmp.c | 49
hmp.h | 1 +
hw/net/virtio-net.c | 93
+
include/net/net.h | 2 ++
monitor.c | 8 +
net/net.c | 47 +++
qapi-schema.json| 73 +
qmp-commands.hx | 55 +++
9 files changed, 330 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..b7863eb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@ show qdev device model list
show roms
@item info tpm
show the TPM device
+@item info rx-filter [net client name]
+show the rx-filter information for all nics (or for the given nic)
Humor me: spell it NICs and NIC, because it's an acronym.
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 4fb76ec..5b47382 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict)
+{
+RxFilterInfoList *filter_list, *entry;
+strList *str_entry;
+bool has_name = qdict_haskey(qdict, name);
+const char *name = NULL;
+
+if (has_name) {
+name = qdict_get_str(qdict, name);
+}
+
+filter_list = qmp_query_rx_filter(has_name, name, NULL);
Rather roundabout way to do
const char *name = qdict_get_str(qdict, name);
filter_list = qmp_query_rx_filter(name != NULL, name);
+entry = filter_list;
+
+while (entry) {
Recommend to put the loop control in one place:
for (entry = filter_list; entry; entry = entry-next) {
+monitor_printf(mon, %s:\n, entry-value-name);
+monitor_printf(mon, \\ promiscuous: %s\n,
+ entry-value-promiscuous ? on : off);
+monitor_printf(mon, \\ multicast: %s\n,
+ RxState_lookup[entry-value-multicast]);
+monitor_printf(mon, \\ unicast: %s\n,
+ RxState_lookup[entry-value-unicast]);
+monitor_printf(mon, \\ broadcast-allowed: %s\n,
+ entry-value-broadcast_allowed ? on : off);
+monitor_printf(mon, \\ multicast-overflow: %s\n,
+ entry-value-multicast_overflow ? on : off);
+monitor_printf(mon, \\ unicast-overflow: %s\n,
+ entry-value-unicast_overflow ? on : off);
+monitor_printf(mon, \\ main-mac: %s\n, entry-value-main_mac);
+
+str_entry = entry-value-unicast_table;
+monitor_printf(mon, \\ unicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+str_entry = entry-value-multicast_table;
+monitor_printf(mon, \\ multicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+entry = entry-next;
+}
+qapi_free_RxFilterInfoList(filter_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..9af733e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..f93e021 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
#include hw/virtio/virtio-net.h
#include net/vhost_net.h
#include hw/virtio/virtio-bus.h
+#include qapi/qmp/qjson.h
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Meh, I got confused and reviewed an out-of-date version. Hope it's not entirely noise.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Wed, 26 Jun 2013 14:00:30 +0200 Markus Armbruster arm...@redhat.com wrote: Meh, I got confused and reviewed an out-of-date version. Hope it's not entirely noise. I don't think it is. But this series got applied to Michael's tree, so if you want your comments addressed before applying to master (I think we do) then it's better to state it clearly.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Luiz Capitulino lcapitul...@redhat.com writes: On Wed, 26 Jun 2013 14:00:30 +0200 Markus Armbruster arm...@redhat.com wrote: Meh, I got confused and reviewed an out-of-date version. Hope it's not entirely noise. I don't think it is. But this series got applied to Michael's tree, so if you want your comments addressed before applying to master (I think we do) then it's better to state it clearly. Michael, please give Amos a chance to reply to my review.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Sun, May 26, 2013 at 10:38:14AM +0300, Michael S. Tsirkin wrote: On Fri, May 24, 2013 at 04:00:42PM -0400, Luiz Capitulino wrote: On Fri, 24 May 2013 12:05:12 -0600 Eric Blake ebl...@redhat.com wrote: On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote: Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. And the time spent malloc'ing the larger message to send from qemu, as well as the time spent malloc'ing the libvirt side that parses the qemu string into C code for use, and the time spent strcmp'ing every entry to find the right one... It really IS more efficient to filter as low down in the stack as possible, once it is determined that filtering is desirable. Whether filtering makes a difference in performance is a different question - you may be right that always returning the entire list and making libvirt do its own filtering will still not add any more noticeable delay compared to libvirt doing a filtered query, if the bottleneck lies elsewhere (such as libvirt telling macvtap its new configration). My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory. For this _particular_ interface, I'm not sure whether libvirt will ever use an unfiltered query - If having the argument is useful for libvirt, then it's fine to have it. But I'd be very reluctant to buy any performance argument w/o real numbers to back them up. Me too. I think it's more convenience than performance. Agreed. I suggested filtering on a NIC for usability rather than performance reasons. QMP should be easy to use. Requiring every client to fish for the right NIC in a bunch of output that gets discarded is not convenient. Stefan
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, May 24, 2013 at 04:00:42PM -0400, Luiz Capitulino wrote: On Fri, 24 May 2013 12:05:12 -0600 Eric Blake ebl...@redhat.com wrote: On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote: Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. And the time spent malloc'ing the larger message to send from qemu, as well as the time spent malloc'ing the libvirt side that parses the qemu string into C code for use, and the time spent strcmp'ing every entry to find the right one... It really IS more efficient to filter as low down in the stack as possible, once it is determined that filtering is desirable. Whether filtering makes a difference in performance is a different question - you may be right that always returning the entire list and making libvirt do its own filtering will still not add any more noticeable delay compared to libvirt doing a filtered query, if the bottleneck lies elsewhere (such as libvirt telling macvtap its new configration). My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory. For this _particular_ interface, I'm not sure whether libvirt will ever use an unfiltered query - If having the argument is useful for libvirt, then it's fine to have it. But I'd be very reluctant to buy any performance argument w/o real numbers to back them up. Me too. I think it's more convenience than performance. -- MST
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, 24 May 2013 11:08:13 +0800
Amos Kong ak...@redhat.com wrote:
On Thu, May 23, 2013 at 12:03:25PM -0400, Luiz Capitulino wrote:
On Thu, 23 May 2013 17:08:00 +0800
Amos Kong ak...@redhat.com wrote:
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
+continue;
+}
I don't think we need this argument. This command is quite simple in its
response, let's do this filtering in HMP only.
Event message contains the net client name, management might only want
to query the single net client.
The client can do the filtering itself.
And we plan to use a flag for _each nic_ to control the event notification,
querying single net client will only clear it's flag.
So we need to support query by net-client-name.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On 05/24/2013 06:23 AM, Luiz Capitulino wrote: I don't think we need this argument. This command is quite simple in its response, let's do this filtering in HMP only. Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. If we're arguing that we want this to be as responsive as possible, then the less data we send over the wire, the faster management can react to the guest's request for a particular NIC. That is, if libvirt is listening to events that says NIC2 wants to change rx-filter, libvirt would rather do a filtered query where it knows the JSON array of 1 element matches NIC2 data, rather than do a global query and search through the returned array until it finds NIC2. Filtering is relatively easy to add, whether you do it in QMP or make every client add it. Libvirt will survive if you don't have filtering, but I don't see why we can't have it in QMP. Also, if you DO decide to rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since the events say which NIC is requesting a change, even if the query reads all nics, libvirt will only change the macvtap settings of the nic(s) for which it has received an event (it doesn't make sense to waste time requesting a (no-op) change to macvtap settings on a nic that hasn't requested a change). But if you argue that having no filtering in the QMP command means that you can get away with a single flag instead of a per-nic flag, then you will fail to emit an event for NIC2 if it changes in between the time that NIC1 fired an event and libvirt finally does the query, and libvirt wouldn't realize that NIC2 also needs a macvtap change. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, 24 May 2013 06:55:44 -0600 Eric Blake ebl...@redhat.com wrote: On 05/24/2013 06:23 AM, Luiz Capitulino wrote: I don't think we need this argument. This command is quite simple in its response, let's do this filtering in HMP only. Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. If we're arguing that we want this to be as responsive as possible, then the less data we send over the wire, the faster management can react to the guest's request for a particular NIC. That is, if libvirt is listening to events that says NIC2 wants to change rx-filter, libvirt would rather do a filtered query where it knows the JSON array of 1 element matches NIC2 data, rather than do a global query and search through the returned array until it finds NIC2. This sounds like premature optimization to me, but I wonder if instead of cluttering commands with arguments to do the filtering we could add some standard way of doing this in the QAPI. It was you who suggested a filter command? Filtering is relatively easy to add, whether you do it in QMP or make every client add it. Libvirt will survive if you don't have filtering, but I don't see why we can't have it in QMP. Also, if you DO decide to rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since the events say which NIC is requesting a change, even if the query reads all nics, libvirt will only change the macvtap settings of the nic(s) for which it has received an event (it doesn't make sense to waste time requesting a (no-op) change to macvtap settings on a nic that hasn't requested a change). But if you argue that having no filtering in the QMP command means that you can get away with a single flag instead of a per-nic flag, then you will fail to emit an event for NIC2 if it changes in between the time that NIC1 fired an event and libvirt finally does the query, and libvirt wouldn't realize that NIC2 also needs a macvtap change.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On 05/24/2013 07:08 AM, Luiz Capitulino wrote: This sounds like premature optimization to me, but I wonder if instead of cluttering commands with arguments to do the filtering we could add some standard way of doing this in the QAPI. Maybe we could make QAPI support generic filtering for all query-* commands. But there's still a cost with making all query-* commands malloc the space for an entire list only to then have a QAPI layer on top of it do filtering before handing back the single element over the wire, compared to passing the filtering down to the query-* implementation to do the filtering in place. It was you who suggested a filter command? No, Stefan suggested it on v1: https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03102.html -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
Eric Blake ebl...@redhat.com writes: On 05/24/2013 06:23 AM, Luiz Capitulino wrote: I don't think we need this argument. This command is quite simple in its response, let's do this filtering in HMP only. Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. If we're arguing that we want this to be as responsive as possible, then the less data we send over the wire, the faster management can react to the guest's request for a particular NIC. That is, if libvirt is listening to events that says NIC2 wants to change rx-filter, libvirt would rather do a filtered query where it knows the JSON array of 1 element matches NIC2 data, rather than do a global query and search through the returned array until it finds NIC2. Filtering is relatively easy to add, whether you do it in QMP or make every client add it. Libvirt will survive if you don't have filtering, but I don't see why we can't have it in QMP. Also, if you DO decide to rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since the events say which NIC is requesting a change, even if the query reads all nics, libvirt will only change the macvtap settings of the nic(s) for which it has received an event (it doesn't make sense to waste time requesting a (no-op) change to macvtap settings on a nic that hasn't requested a change). But if you argue that having no filtering in the QMP command means that you can get away with a single flag instead of a per-nic flag, then you will fail to emit an event for NIC2 if it changes in between the time that NIC1 fired an event and libvirt finally does the query, and libvirt wouldn't realize that NIC2 also needs a macvtap change. No disagreement on the need for a per-NIC flag. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, May 24, 2013 at 05:20:13PM +0200, Markus Armbruster wrote: Eric Blake ebl...@redhat.com writes: On 05/24/2013 06:23 AM, Luiz Capitulino wrote: I don't think we need this argument. This command is quite simple in its response, let's do this filtering in HMP only. Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. If we're arguing that we want this to be as responsive as possible, then the less data we send over the wire, the faster management can react to the guest's request for a particular NIC. That is, if libvirt is listening to events that says NIC2 wants to change rx-filter, libvirt would rather do a filtered query where it knows the JSON array of 1 element matches NIC2 data, rather than do a global query and search through the returned array until it finds NIC2. Filtering is relatively easy to add, whether you do it in QMP or make every client add it. Libvirt will survive if you don't have filtering, but I don't see why we can't have it in QMP. Also, if you DO decide to rip filtering out of QMP, you STILL need to keep a per-NIC flag. Since the events say which NIC is requesting a change, even if the query reads all nics, libvirt will only change the macvtap settings of the nic(s) for which it has received an event (it doesn't make sense to waste time requesting a (no-op) change to macvtap settings on a nic that hasn't requested a change). But if you argue that having no filtering in the QMP command means that you can get away with a single flag instead of a per-nic flag, then you will fail to emit an event for NIC2 if it changes in between the time that NIC1 fired an event and libvirt finally does the query, and libvirt wouldn't realize that NIC2 also needs a macvtap change. No disagreement on the need for a per-NIC flag. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory. We need the query for macvtap devices. We don't need it for tap devices. In that case you don't want tap device info. Maybe some libvirt guys can tell us whether they prefer a per device query or a global one with info for all NICs? I think for HMP it's best to have nic optional. Is it a good idea to make QMP match HMP closely? -- MST
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote: Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. And the time spent malloc'ing the larger message to send from qemu, as well as the time spent malloc'ing the libvirt side that parses the qemu string into C code for use, and the time spent strcmp'ing every entry to find the right one... It really IS more efficient to filter as low down in the stack as possible, once it is determined that filtering is desirable. Whether filtering makes a difference in performance is a different question - you may be right that always returning the entire list and making libvirt do its own filtering will still not add any more noticeable delay compared to libvirt doing a filtered query, if the bottleneck lies elsewhere (such as libvirt telling macvtap its new configration). My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory. For this _particular_ interface, I'm not sure whether libvirt will ever use an unfiltered query - that is, the rx-filter query will probably always be invoked in response to an event, at which point libvirt only cares about the filter status of the nic named in the event. And ultimately libvirt knows what nics it passed to the guest, so even if there isn't a global query and I guessed wrong about libvirt never wanting all state at once, it would still be possible for libvirt to iterate over one query per nic. On the other hand, consistency with other query-* QMP commands says that most of them return as much information as possible all the time, and generally libvirt likes this - even the newly-added query-command-line-options has a filtering option, but current libvirt.git only uses it once in global mode rather than once-per-option in filtered mode. We need the query for macvtap devices. We don't need it for tap devices. In that case you don't want tap device info. Maybe some libvirt guys can tell us whether they prefer a per device query or a global one with info for all NICs? Libvirt can cope either way. I personally like the idea of allowing both global and filtered queries, without second-guessing what management apps will prefer to use, and don't think filtering adds that much complexity. But if you want to insist on avoiding filtering, I'd rather have a global query than a mandatory name argument, for consistency with other query-* commands, even if libvirt then ends up doing its own filtering. If we get introspection into qemu 1.6 at the same time as the new query for rx-filters, it really won't matter whether you start with global-only or mandatory name-only; either way, if we change our mind and add the other mode in qemu 1.7, libvirt will still be able to use introspection to determine whether the argument is present in one direction (going from global-only to optional filtering), or whether the argument has been made optionl in the other direction (going from mandatory name to optional global). I think for HMP it's best to have nic optional. This is true, no matter what we decide for QMP. Is it a good idea to make QMP match HMP closely? QMP has to provide enough information for HMP to do its job. How will HMP do global listing if QMP doesn't provide a way to get all the devices at once? Remember, libvirt knows what devices it told qemu to create, but I don't know that HMP has the same visibility into the list of possible devices that can be queried. So you may need a global mode to begin with. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Fri, 24 May 2013 12:05:12 -0600 Eric Blake ebl...@redhat.com wrote: On 05/24/2013 10:12 AM, Michael S. Tsirkin wrote: Event message contains the net client name, management might only want to query the single net client. The client can do the filtering itself. I'm not sure I buy the responsiveness argument. Sure, the fastest I/O is no I/O, but whether you read and parse 100 bytes or 1000 from a Unix domain socket once in a great while shouldn't make a difference. And the time spent malloc'ing the larger message to send from qemu, as well as the time spent malloc'ing the libvirt side that parses the qemu string into C code for use, and the time spent strcmp'ing every entry to find the right one... It really IS more efficient to filter as low down in the stack as possible, once it is determined that filtering is desirable. Whether filtering makes a difference in performance is a different question - you may be right that always returning the entire list and making libvirt do its own filtering will still not add any more noticeable delay compared to libvirt doing a filtered query, if the bottleneck lies elsewhere (such as libvirt telling macvtap its new configration). My main concern is to keep the external interface simple. I'm rather reluctant to have query commands grow options. In a case where we need the give me everything query anyway, the give me this particular part option is additional complexity. Needs justification, say arguments involving throughput, latency or client complexity. Perhaps cases exist where we never want to ask for everything. Then the give me everything query is useless, and the option should be mandatory. For this _particular_ interface, I'm not sure whether libvirt will ever use an unfiltered query - If having the argument is useful for libvirt, then it's fine to have it. But I'd be very reluctant to buy any performance argument w/o real numbers to back them up.
[Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
---
hmp-commands.hx | 2 ++
hmp.c | 49
hmp.h | 1 +
hw/net/virtio-net.c | 93 +
include/net/net.h | 2 ++
monitor.c | 8 +
net/net.c | 47 +++
qapi-schema.json| 73 +
qmp-commands.hx | 55 +++
9 files changed, 330 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..b7863eb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@ show qdev device model list
show roms
@item info tpm
show the TPM device
+@item info rx-filter [net client name]
+show the rx-filter information for all nics (or for the given nic)
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 4fb76ec..5b47382 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict)
+{
+RxFilterInfoList *filter_list, *entry;
+strList *str_entry;
+bool has_name = qdict_haskey(qdict, name);
+const char *name = NULL;
+
+if (has_name) {
+name = qdict_get_str(qdict, name);
+}
+
+filter_list = qmp_query_rx_filter(has_name, name, NULL);
+entry = filter_list;
+
+while (entry) {
+monitor_printf(mon, %s:\n, entry-value-name);
+monitor_printf(mon, \\ promiscuous: %s\n,
+ entry-value-promiscuous ? on : off);
+monitor_printf(mon, \\ multicast: %s\n,
+ RxState_lookup[entry-value-multicast]);
+monitor_printf(mon, \\ unicast: %s\n,
+ RxState_lookup[entry-value-unicast]);
+monitor_printf(mon, \\ broadcast-allowed: %s\n,
+ entry-value-broadcast_allowed ? on : off);
+monitor_printf(mon, \\ multicast-overflow: %s\n,
+ entry-value-multicast_overflow ? on : off);
+monitor_printf(mon, \\ unicast-overflow: %s\n,
+ entry-value-unicast_overflow ? on : off);
+monitor_printf(mon, \\ main-mac: %s\n, entry-value-main_mac);
+
+str_entry = entry-value-unicast_table;
+monitor_printf(mon, \\ unicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+str_entry = entry-value-multicast_table;
+monitor_printf(mon, \\ multicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+entry = entry-next;
+}
+qapi_free_RxFilterInfoList(filter_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..9af733e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..f93e021 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
#include hw/virtio/virtio-net.h
#include net/vhost_net.h
#include hw/virtio/virtio-bus.h
+#include qapi/qmp/qjson.h
+#include monitor/monitor.h
#define VIRTIO_NET_VM_VERSION11
@@ -192,6 +194,90 @@ static void virtio_net_set_link_status(NetClientState *nc)
virtio_net_set_status(vdev, vdev-status);
}
+static bool notify_enabled = true;
+
+static void rxfilter_notify(const char *name)
+{
+QObject *event_data;
+
+if (notify_enabled) {
+event_data = qobject_from_jsonf({ 'name': %s }, name);
+monitor_protocol_event(QEVENT_RX_FILTER_CHANGED, event_data);
+
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, May 23, 2013 at 05:08:00PM +0800, Amos Kong wrote:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
---
hmp-commands.hx | 2 ++
hmp.c | 49
hmp.h | 1 +
hw/net/virtio-net.c | 93
+
include/net/net.h | 2 ++
monitor.c | 8 +
net/net.c | 47 +++
qapi-schema.json| 73 +
qmp-commands.hx | 55 +++
9 files changed, 330 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..b7863eb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@ show qdev device model list
show roms
@item info tpm
show the TPM device
+@item info rx-filter [net client name]
+show the rx-filter information for all nics (or for the given nic)
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 4fb76ec..5b47382 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict)
+{
+RxFilterInfoList *filter_list, *entry;
+strList *str_entry;
+bool has_name = qdict_haskey(qdict, name);
+const char *name = NULL;
+
+if (has_name) {
+name = qdict_get_str(qdict, name);
+}
+
+filter_list = qmp_query_rx_filter(has_name, name, NULL);
+entry = filter_list;
+
+while (entry) {
+monitor_printf(mon, %s:\n, entry-value-name);
+monitor_printf(mon, \\ promiscuous: %s\n,
+ entry-value-promiscuous ? on : off);
+monitor_printf(mon, \\ multicast: %s\n,
+ RxState_lookup[entry-value-multicast]);
+monitor_printf(mon, \\ unicast: %s\n,
+ RxState_lookup[entry-value-unicast]);
+monitor_printf(mon, \\ broadcast-allowed: %s\n,
+ entry-value-broadcast_allowed ? on : off);
+monitor_printf(mon, \\ multicast-overflow: %s\n,
+ entry-value-multicast_overflow ? on : off);
+monitor_printf(mon, \\ unicast-overflow: %s\n,
+ entry-value-unicast_overflow ? on : off);
+monitor_printf(mon, \\ main-mac: %s\n, entry-value-main_mac);
+
+str_entry = entry-value-unicast_table;
+monitor_printf(mon, \\ unicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+str_entry = entry-value-multicast_table;
+monitor_printf(mon, \\ multicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+entry = entry-next;
+}
+qapi_free_RxFilterInfoList(filter_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..9af733e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..f93e021 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
#include hw/virtio/virtio-net.h
#include net/vhost_net.h
#include hw/virtio/virtio-bus.h
+#include qapi/qmp/qjson.h
+#include monitor/monitor.h
#define VIRTIO_NET_VM_VERSION11
@@ -192,6 +194,90 @@ static void virtio_net_set_link_status(NetClientState
*nc)
virtio_net_set_status(vdev, vdev-status);
}
+static bool notify_enabled = true;
Please, make this part of the device
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On 05/23/2013 03:08 AM, Amos Kong wrote:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
s/don't/doesn't/
rx-filter after receives one event, new events won't be sent
s/after/after it/
to qmp monitor.
+++ b/net/net.c
@@ -961,6 +961,53 @@ void print_net_client(Monitor *mon, NetClientState *nc)
nc-info_str);
}
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
Do you need the has_name argument here, or can you ensure that the
caller passes NULL when the caller's has_name was false, for one less
parameter and the same amount of information?
+++ b/qapi-schema.json
@@ -3619,3 +3619,76 @@
'*cpuid-input-ecx': 'int',
'cpuid-register': 'X86CPURegister32',
'features': 'int' } }
+
+##
+# @RxState:
+#
+# Packets receiving state
+#
+# @normal: filter assigned packets according to the mac-table
+#
+# @no: don't receive any assigned packet
+#
+# @all: receive all assigned packets
+#
+##
+{ 'enum': 'RxState', 'data': [ 'normal', 'no', 'all' ] }
I think s/no/none/ would make slightly more sense (usually, you pair
no/yes and none/all, not no/all).
+
+##
+# @RxFilterInfo:
+#
+# Rx-filter information for a net client, it contains main mac, some
+# of rx-mode items and mac-table.
+#
+# @name: net client name
+#
+# @promiscuous: whether to ether promiscuous mode
s/to ether//; s/$/is enabled/
+#
+# @multicast: multicast receive state
+#
+# @unicast: unicast receive state
+#
+# @broadcast-allowed: whether to receive broadcast
+#
+# @multicast-overflow: multicast table is overflow or not
+#
+# @unicast-overflow: unicast table is overflow or not
+#
+# @main-mac: the main macaddr string
+#
+# @unicast-table: a list of unicast macaddr string
+#
+# @multicast-table: a list of multicast macaddr string
Naming is reasonable; thanks for improving things from v1.
+#
+# Since 1.6
+##
+
+{ 'type': 'RxFilterInfo',
+ 'data': {
+'name': 'str',
+'promiscuous':'bool',
+'multicast': 'RxState',
+'unicast':'RxState',
+'broadcast-allowed': 'bool',
+'multicast-overflow': 'bool',
+'unicast-overflow': 'bool',
+'main-mac': 'str',
+'unicast-table': ['str'],
+'multicast-table':['str'] }}
+
+##
+# @query-rx-filter:
+#
+# Return rx-filter information for all nics (or for the given nic).
+#
+# @name: #optional net client name
+#
+# Returns: list of @RxFilterInfo for all nics (or for the given nic).
+# Returns an error if the given @name doesn't exist, or given
+# nic doesn't support rx-filter querying, or no net client
+# supports rx-filter querying
+#
+# Since: 1.6
+##
+{ 'command': 'query-rx-filter', 'data': { '*name': 'str' },
+ 'returns': ['RxFilterInfo'] }
Interface looks reasonable. I didn't check the code closely, but agree
with Michael's assessment that the event-suppression flag has to be
per-device, not global.
+
+Each array entry contains the following:
+
+- name: net client name (jaso-string)
+- promiscuous: enter promiscuous mode (json-bool)
+- multicast: multicast receive state (one of 'normal', 'no', 'all')
+- unicast: unicast receive state (one of 'normal', 'no', 'all')
+- broadcast-allowed: allow to receive broadcast (json-bool)
+- multicast-overflow: multicast table is overflow (json-bool)
s/is overflow/overflowed/
+- unicast-overflow: unicast table is overflow (json-bool)
and again
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, 23 May 2013 17:08:00 +0800
Amos Kong ak...@redhat.com wrote:
We want to implement mac programming over macvtap through Libvirt,
related rx-filter configuration contains main mac, some of rx-mode
and mac-table.
The previous patch adds QMP event to notify management of rx-filter
change. This patch adds a monitor command to query rx-filter
information.
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
(qemu) info rx-filter vnet0
vnet0:
\ promiscuous: on
\ multicast: normal
\ unicast: normal
\ broadcast-allowed: off
\ multicast-overflow: off
\ unicast-overflow: off
\ main-mac: 52:54:00:12:34:56
\ unicast-table:
\ multicast-table:
01:00:5e:00:00:01
33:33:00:00:00:01
33:33:ff:12:34:56
Signed-off-by: Amos Kong ak...@redhat.com
Looks good in general. I have some comments below and restarted the
dicussion about notify_enabled.
---
hmp-commands.hx | 2 ++
hmp.c | 49
hmp.h | 1 +
hw/net/virtio-net.c | 93
+
include/net/net.h | 2 ++
monitor.c | 8 +
net/net.c | 47 +++
qapi-schema.json| 73 +
qmp-commands.hx | 55 +++
9 files changed, 330 insertions(+)
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 9cea415..b7863eb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1639,6 +1639,8 @@ show qdev device model list
show roms
@item info tpm
show the TPM device
+@item info rx-filter [net client name]
+show the rx-filter information for all nics (or for the given nic)
@end table
ETEXI
diff --git a/hmp.c b/hmp.c
index 4fb76ec..5b47382 100644
--- a/hmp.c
+++ b/hmp.c
@@ -653,6 +653,55 @@ void hmp_info_tpm(Monitor *mon, const QDict *qdict)
qapi_free_TPMInfoList(info_list);
}
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict)
+{
+RxFilterInfoList *filter_list, *entry;
+strList *str_entry;
+bool has_name = qdict_haskey(qdict, name);
+const char *name = NULL;
+
+if (has_name) {
+name = qdict_get_str(qdict, name);
+}
+
+filter_list = qmp_query_rx_filter(has_name, name, NULL);
qmp_query_rx_filter() can fail.
+entry = filter_list;
+
+while (entry) {
+monitor_printf(mon, %s:\n, entry-value-name);
+monitor_printf(mon, \\ promiscuous: %s\n,
+ entry-value-promiscuous ? on : off);
+monitor_printf(mon, \\ multicast: %s\n,
+ RxState_lookup[entry-value-multicast]);
+monitor_printf(mon, \\ unicast: %s\n,
+ RxState_lookup[entry-value-unicast]);
+monitor_printf(mon, \\ broadcast-allowed: %s\n,
+ entry-value-broadcast_allowed ? on : off);
+monitor_printf(mon, \\ multicast-overflow: %s\n,
+ entry-value-multicast_overflow ? on : off);
+monitor_printf(mon, \\ unicast-overflow: %s\n,
+ entry-value-unicast_overflow ? on : off);
+monitor_printf(mon, \\ main-mac: %s\n, entry-value-main_mac);
+
+str_entry = entry-value-unicast_table;
+monitor_printf(mon, \\ unicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
+
+str_entry = entry-value-multicast_table;
+monitor_printf(mon, \\ multicast-table:\n);
+while (str_entry) {
+monitor_printf(mon, %s\n, str_entry-value);
+str_entry = str_entry-next;
+}
Can't that loop be moved to a function?
+
+entry = entry-next;
+}
+qapi_free_RxFilterInfoList(filter_list);
+}
+
void hmp_quit(Monitor *mon, const QDict *qdict)
{
monitor_suspend(mon);
diff --git a/hmp.h b/hmp.h
index 95fe76e..9af733e 100644
--- a/hmp.h
+++ b/hmp.h
@@ -37,6 +37,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict);
void hmp_info_pci(Monitor *mon, const QDict *qdict);
void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
void hmp_info_tpm(Monitor *mon, const QDict *qdict);
+void hmp_info_rx_filter(Monitor *mon, const QDict *qdict);
void hmp_quit(Monitor *mon, const QDict *qdict);
void hmp_stop(Monitor *mon, const QDict *qdict);
void hmp_system_reset(Monitor *mon, const QDict *qdict);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 1ea9556..f93e021 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -21,6 +21,8 @@
#include hw/virtio/virtio-net.h
#include net/vhost_net.h
#include hw/virtio/virtio-bus.h
+#include qapi/qmp/qjson.h
+#include monitor/monitor.h
#define VIRTIO_NET_VM_VERSION11
@@ -192,6 +194,90 @@
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, May 23, 2013 at 06:14:19AM -0600, Eric Blake wrote:
On 05/23/2013 03:08 AM, Amos Kong wrote:
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
Do you need the has_name argument here, or can you ensure that the
caller passes NULL when the caller's has_name was false,
hmp_info_rx_filter() passes NULL name when has_name is false.
'has_name' is need here. Or we can change it to:
if (name strcmp(nc-name, name) != 0) {
I think using 'has_name' is clearer.
for one less parameter and the same amount of information?
qmp_query_rx_filter() define is generated by QAPI infrastructure,
the parameters are fixed. We also use it in hmp_info_rx_filter().
--
Amos.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, May 23, 2013 at 12:03:25PM -0400, Luiz Capitulino wrote:
On Thu, 23 May 2013 17:08:00 +0800
Amos Kong ak...@redhat.com wrote:
A flag is used to avoid events flooding, if user don't query
rx-filter after receives one event, new events won't be sent
to qmp monitor.
+RxFilterInfoList *qmp_query_rx_filter(bool has_name, const char *name,
+ Error **errp)
+{
+NetClientState *nc;
+RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
+
+QTAILQ_FOREACH(nc, net_clients, next) {
+RxFilterInfoList *entry;
+RxFilterInfo *info;
+
+if (nc-info-type != NET_CLIENT_OPTIONS_KIND_NIC) {
+continue;
+}
+if (has_name strcmp(nc-name, name) != 0) {
+continue;
+}
I don't think we need this argument. This command is quite simple in its
response, let's do this filtering in HMP only.
Event message contains the net client name, management might only want
to query the single net client.
And we plan to use a flag for _each nic_ to control the event notification,
querying single net client will only clear it's flag.
So we need to support query by net-client-name.
--
Amos.
Re: [Qemu-devel] [PATCH v3 2/2] net: introduce command to query rx-filter information
On Thu, May 23, 2013 at 01:23:40PM +0300, Michael S. Tsirkin wrote:
On Thu, May 23, 2013 at 05:08:00PM +0800, Amos Kong wrote:
+info-broadcast_allowed = n-nobcast;
+info-multicast_overflow = n-mac_table.multi_overflow;
+info-unicast_overflow = n-mac_table.uni_overflow;
+info-main_mac = g_strdup_printf(%.2x:%.2x:%.2x:%.2x:%.2x:%.2x,
+ n-mac[0], n-mac[1], n-mac[2],
+ n-mac[3], n-mac[4], n-mac[5]);
We really want a helper for this g_strdup_printf thing IMO.
entry-value = mac_strdup_printf(n-mac_table.macs + i * ETH_ALEN);
static char *mac_strdup_printf(uint8_t *mac)
{
return g_strdup_printf(%.2x:%.2x:%.2x:%.2x:%.2x:%.2x, mac[0],
mac[1], mac[2], mac[3], mac[4], mac[5]);
}
--
Amos.
