I'm a bit late to the thread but I have a few other comments below.
I'd like to get this patch in the next pull request if possible so I'd
appreciate if others can give any comments on the patch also.
Thanks
Ian
Signed-off-by: Robert Mulik <robert.mu...@ericsson.com>
---
v5 -> v6:
- DPDK install documentation updated.
- Status of lsc_interrupt_mode of DPDK interfaces can be read by
command
ovs-appctl dpif/show.
- It was suggested to check if the HW supports interrupt mode, but it
is not
possible to do without DPDK code change, so it is skipped from this
patch.
---
Documentation/intro/install/dpdk.rst | 33
+++++++++++++++++++++++++++++++++
lib/netdev-dpdk.c | 24 ++++++++++++++++++++++--
vswitchd/vswitch.xml | 17 +++++++++++++++++
3 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/Documentation/intro/install/dpdk.rst
b/Documentation/intro/install/dpdk.rst
index ed358d5..eb1bc7b 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -628,6 +628,39 @@ The average number of packets per output batch
can be checked in PMD stats::
$ ovs-appctl dpif-netdev/pmd-stats-show
+Link State Change (LSC) detection configuration
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+There are two methods to get the information when Link State Change
+(LSC) happens on a network interface: by polling or interrupt.
+
+With the polling method, the main process checks the link state on a
+fixed interval. This fixed interval determines how fast a link
+change is detected.
+
+If interrupts are used to get LSC information, the hardware itself
+triggers an interrupt when link state change happens, the interrupt
+thread wakes up from sleep, updates the information, and goes back
+to sleep mode. When no link state change happens (most of the time),
+the thread remains in sleep mode and doesn`t use processor time at
+all. The disadvantage of this method is that when an interrupt
+happens, the processor has to handle it immediately, so it puts the
+currently running process to background, handles the interrupt, and
+takes the
background process back.
+
+Note that not all PMD drivers support LSC interrupts.
+
+The default configuration is polling mode. To set interrupt mode,
+option ``dpdk-lsc-interrupt`` has to be set to ``true``.
+
+Command to set interrupt mode for a specific interface::
+ $ ovs-vsctl set interface <iface_name>
+options:dpdk-lsc-interrupt=true
+
+Command to set polling mode for a specific interface::
+ $ ovs-vsctl set interface <iface_name>
+options:dpdk-lsc-interrupt=false
+
+Command to remove ``dpdk-lsc-interrupt`` option::
+ $ ovs-vsctl remove interface <iface_name> options
+dpdk-lsc-interrupt
Just a query, why do we need the above option to remove lsc, is setting
lsc true or false with the previous commands not enough?
+
Limitations
------------
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
94fb163..e2794e8
100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -433,6 +433,12 @@ struct netdev_dpdk {
/* DPDK-ETH hardware offload features,
* from the enum set 'dpdk_hw_ol_features' */
uint32_t hw_ol_features;
+
+ /* Properties for link state change detection mode.
+ * If lsc_interrupt_mode is set to false, poll mode is used,
+ * otherwise interrupt mode is used. */
+ bool requested_lsc_interrupt_mode;
+ bool lsc_interrupt_mode;
);
PADDED_MEMBERS(CACHE_LINE_SIZE,
@@ -686,12 +692,14 @@ dpdk_watchdog(void *dummy OVS_UNUSED) }
static int
-dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int
n_txq)
+dpdk_eth_dev_port_config(struct netdev_dpdk *dev, int n_rxq, int
+n_txq)
{
int diag = 0;
int i;
struct rte_eth_conf conf = port_conf;
+ conf.intr_conf.lsc = dev->lsc_interrupt_mode;
Should above be dev->requested_lsc_interrupt_mode? Similar to how we
request MTUs, we first have to validate if it's supported.
Since we have no way of knowing if the interrupt mode is supported at
the moment we should set conf.intr_conf.lsc = dev-
requested_lsc_interrupt_mode and once configuration succeeds set dev-
lsc_interrupt_mode = conf.intr_conf.lsc.
If we don't then I'm not sure of the purpose of having
requested_lsc_interrupt_mode and a separate lsc_interrupt_mode for dev.
Thoughts?
+
/* For some NICs (e.g. Niantic), scatter_rx mode needs to be
explicitly
* enabled. */
if (dev->mtu > ETHER_MTU) {
@@ -801,7 +809,7 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
- diag = dpdk_eth_dev_queue_setup(dev, n_rxq, n_txq);
+ diag = dpdk_eth_dev_port_config(dev, n_rxq, n_txq);
if (diag) {
VLOG_ERR("Interface %s(rxq:%d txq:%d) configure error: %s",
dev->up.name, n_rxq, n_txq, rte_strerror(-diag));
With the change from dpdk_eth_dev_queue_setup to
dpdk_eth_dev_port_config I think we need to improve the error message also
logged above.
Instead of only reporting the rxq and txq we should include the
requested lsc mode also as that could cause a failure if not supported.
@@ -
897,6 +905,7 @@ common_construct(struct netdev *netdev, dpdk_port_t
port_no,
dev->flags = 0;
dev->requested_mtu = ETHER_MTU;
dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
+ dev->requested_lsc_interrupt_mode = 0;
ovsrcu_index_init(&dev->vid, -1);
dev->vhost_reconfigured = false;
dev->attached = false;
@@ -1320,6 +1329,8 @@ netdev_dpdk_get_config(const struct netdev
*netdev, struct smap *args)
} else {
smap_add(args, "rx_csum_offload", "false");
}
+ smap_add(args, "lsc_interrupt_mode",
+ dev->lsc_interrupt_mode?"true":"false");
Need space before/after operators '?' and ':' above.
}
ovs_mutex_unlock(&dev->mutex);
@@ -1520,6 +1531,12 @@ netdev_dpdk_set_config(struct netdev *netdev,
const struct smap *args,
goto out;
}
+ bool lsc_interrupt_mode = smap_get_bool(args,
+ "dpdk-lsc-interrupt",
This comes down to style preference but can you declare this bool at the
beginning of the function? There are other bool variable in the function
declared there already so it keeps with the existing style of the
function.
+1
false);
+ if (dev->requested_lsc_interrupt_mode != lsc_interrupt_mode) {
+ dev->requested_lsc_interrupt_mode = lsc_interrupt_mode;
+ netdev_request_reconfigure(netdev);
+ }
+
rx_fc_en = smap_get_bool(args, "rx-flow-ctrl", false);
tx_fc_en = smap_get_bool(args, "tx-flow-ctrl", false);
autoneg = smap_get_bool(args, "flow-ctrl-autoneg", false); @@
-3546,6
+3563,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
if (netdev->n_txq == dev->requested_n_txq
&& netdev->n_rxq == dev->requested_n_rxq
&& dev->mtu == dev->requested_mtu
+ && dev->lsc_interrupt_mode ==
+ dev->requested_lsc_interrupt_mode
&& dev->rxq_size == dev->requested_rxq_size
&& dev->txq_size == dev->requested_txq_size
&& dev->socket_id == dev->requested_socket_id) { @@ -3561,6
+3579,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
goto out;
}
+ dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode;
I think this would be removed from here and only set once
rte_eth_dev_configure succeeds in dpdk_eth_dev_port_config as mentioned
earlier?
+
netdev->n_txq = dev->requested_n_txq;
netdev->n_rxq = dev->requested_n_rxq;
It could be useful to report the LSC mode configured for the port also
as part of the port status, something like
@@ -2781,6 +2783,8 @@ netdev_dpdk_get_status(const struct netdev
*netdev, struct smap *args)
dev_info.max_hash_mac_addrs);
smap_add_format(args, "max_vfs", "%u", dev_info.max_vfs);
smap_add_format(args, "max_vmdq_pools", "%u",
dev_info.max_vmdq_pools);
+ smap_add_format(args, "lsc_mode", "%s",
+ dev->lsc_interrupt_mode ? "Interrupt" :
+ "PMD");
Thoughts?
We have this in 'netdev_dpdk_get_config()'. Do you think we need this in
two places? Also, 'netdev_dpdk_get_status()' is more like HW specific
invariants. (Not sure why we're placing these values in 'status', it's a
bit strange.)
