> -----Original Message----- > From: Eelco Chaudron [mailto:[email protected]] > Sent: Wednesday, February 21, 2018 1:29 PM > To: Róbert Mulik <[email protected]>; [email protected] > Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC) > detection mode > > On 21/02/18 11:33, Róbert Mulik wrote: > > > >> -----Original Message----- > >> From: [email protected] [mailto:ovs-dev- > >> [email protected]] On Behalf Of Eelco Chaudron > >> Sent: Tuesday, February 20, 2018 3:17 PM > >> To: [email protected] > >> Subject: Re: [ovs-dev] [PATCH v5] Configurable Link State Change (LSC) > >> detection mode > >> > >> Hi Robert, > >> > >> I did not see a reply to my v4 reply, so I ask this question again: > >> Did you try what happens if the PMD does not support LSC and its > >> enabled? > > Unfortunately I don't have any cards which don't support LSC, so I couldn't > test it > Guess you could change the code for the card you have to return an > error, or not return it as a supported option. > See my earlier command to get the port supported option and error out > with an error that the card does not support it. You suggested to check the flag RTE_ETH_DEV_INTR_LSC to see if the NIC supports the LSC interrupt mode. I couldn't find a way to check this flag with the current OVS implementation. As I see the DPDK code has to be changed to support it. Do you have any suggestion what to do now? Or did I miss something, and the current implementation can see this flag? > >> Also, it would be easier to understand what you are going to change if > >> you answer the questions in the review emails. > >> > >> For example, this patch is missing the suggestion on adding the LSC > >> config in netdev_dpdk_get_config(), as suggested by Ilya. > > As I understand the reason for this function would be to avoid the > confusion when > > both global and local configs are configured for an interface, especially > > if we > go with > > the implementation where the global config doesn't take effect without > service > > restart. > > > > Since the global config is not implemented in this patch and the default > value remains > > the same as it always was (poll mode) and also the documentation tells > what the default > > value is, I don't really see the importance of this implementation. > I have to disagree here, as the getdev_dpdk_get_config() has all the > configurable options, and you are adding one. > So please add it. > > >> I did a quick visual review below, and will wait for v6 with the above > >> change, and do another test run. > >> > >> //Eelco > >> > >> > >> On 15/02/18 15:44, Róbert Mulik wrote: > >>> It is possible to change LSC detection mode to polling or interrupt mode > >>> for DPDK interfaces. The default is polling mode. To set interrupt mode, > >>> option dpdk-lsc-interrupt has to be set to true. > >>> > >>> In polling mode more processor time is needed, since the OVS > repeatedly > >> reads > >>> the link state with a short period. It can lead to packet loss for certain > >>> systems. > >>> > >>> In interrupt mode the hardware itself triggers an interrupt when link > state > >>> change happens, so less processing time needs for the OVS. > >>> > >>> For detailed description and usage see the dpdk install documentation. > >>> > >>> Signed-off-by: Robert Mulik <[email protected]> > >>> --- > >>> Documentation/intro/install/dpdk.rst | 35 > >> +++++++++++++++++++++++++++++++++++ > >>> lib/netdev-dpdk.c | 22 ++++++++++++++++++++-- > >>> 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..e3872e7 100644 > >>> --- a/Documentation/intro/install/dpdk.rst > >>> +++ b/Documentation/intro/install/dpdk.rst > >>> @@ -628,6 +628,41 @@ 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. Another problem with the poll mode is that on some > hardware > >> a > >>> +polling cycle takes too much time, which (in the end) leads to packet > loss > >>> +for certain systems. > >> Maybe we should stick to facts of the implementation, and not known > >> bugs? So remove the "Another problem..." > >>> + > >>> +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 > >>> + > >>> Limitations > >>> ------------ > >>> > >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >>> index 94fb163..d092ef1 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; > >>> + > >>> /* 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)); > >>> @@ -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; > >>> @@ -1520,6 +1529,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", > >> 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 +3561,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 +3577,8 @@ netdev_dpdk_reconfigure(struct netdev > >> *netdev) > >>> goto out; > >>> } > >>> > >>> + dev->lsc_interrupt_mode = dev->requested_lsc_interrupt_mode; > >>> + > >>> netdev->n_txq = dev->requested_n_txq; > >>> netdev->n_rxq = dev->requested_n_rxq; > >>> > >>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > >>> index 0c6a43d..3c9e637 100644 > >>> --- a/vswitchd/vswitch.xml > >>> +++ b/vswitchd/vswitch.xml > >>> @@ -3631,6 +3631,23 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 > >> type=patch options:peer=p1 \ > >>> </column> > >>> </group> > >>> > >>> + <group title="Link State Change detection mode"> > >>> + <column name="options" key="dpdk-lsc-interrupt" > >>> + type='{"type": "boolean"}'> > >>> + <p> > >>> + Set this value to <code>true</code> to configure interrupt mode > for > >>> + Link State Change (LSC) detection instead of poll mode for the > DPDK > >>> + interface. > >>> + </p> > >>> + <p> > >>> + If this value is not set, poll mode is configured. > >>> + </p> > >>> + <p> > >>> + This parameter has an effect only on netdev dpdk interfaces. > >>> + </p> > >>> + </column> > >>> + </group> > >>> + > >>> <group title="Common Columns"> > >>> The overall purpose of these columns is described under > >> <code>Common > >>> Columns</code> at the beginning of this document. > >>> -- > >>> 1.9.1 > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> [email protected] > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > >> _______________________________________________ > >> dev mailing list > >> [email protected] > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
