[ovs-dev] [ovs-dev, ovs-dev, v4] netdev-dpdk: Configurable Link State Change (LSC) detection mode

Jan Scheurich jan.scheurich at ericsson.com
Wed Feb 7 12:00:09 UTC 2018


I support the idea to remove the global default configuration option. It causes more confusion that it helps. 
If at all, new physical ports are typically added to OVS manually and LSC interrupt mode can be configured as required.

Configurable defaults can make sense for vhostuser ports as they are created automatically from OpenStack.  The operator wants to control default settings per port in the absence of specific configuration coming from OpenStack.

Regards, Jan

> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Eelco Chaudron
> Sent: Tuesday, 06 February, 2018 17:30
> To: Ilya Maximets <i.maximets at samsung.com>; Róbert Mulik <robert.mulik at ericsson.com>; dev at openvswitch.org
> Subject: Re: [ovs-dev] [ovs-dev, ovs-dev, v4] netdev-dpdk: Configurable Link State Change (LSC) detection mode
> 
> On 06/02/18 16:01, Ilya Maximets wrote:
> > On 06.02.2018 16:47, Eelco Chaudron wrote:
> >> On 06/02/18 13:12, Ilya Maximets wrote:
> >>> On 05.02.2018 19:29, Eelco Chaudron wrote:
> >>>> On 02/02/18 17:17, Ilya Maximets wrote:
> >>>>> On 02.02.2018 17:05, 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. It is not possible
> >>>>>> to enable the interrupt mode on all hardware.
> >>>>>>
> >>>>>> For detailed description and usage see the dpdk install documentation.
> >>>>>>
> >>>>>> Signed-off-by: Robert Mulik <robert.mulik at ericsson.com>
> >>>>>> Reviewed-by: Ilya Maximets <i.maximets at samsung.com>
> >>>>> 8<---- SNIP ---->8
> >>>>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> >>>>>> index f44f950..87608a5 100644
> >>>>>> --- a/vswitchd/bridge.c
> >>>>>> +++ b/vswitchd/bridge.c
> >>>>>> @@ -68,6 +68,9 @@
> >>>>>>     #include "lib/vswitch-idl.h"
> >>>>>>     #include "xenserver.h"
> >>>>>>     #include "vlan-bitmap.h"
> >>>>>> +#ifdef DPDK_NETDEV
> >>>>>> +#include "./lib/netdev-provider.h"
> >>>>>> +#endif
> >>>>>>
> >>>>>>     VLOG_DEFINE_THIS_MODULE(bridge);
> >>>>>>
> >>>>>> @@ -601,6 +604,11 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
> >>>>>>         ofproto_set_vlan_limit(smap_get_int(&ovs_cfg->other_config, "vlan-limit",
> >>>>>>                                            LEGACY_MAX_VLAN_HEADERS));
> >>>>>>
> >>>>>> +#ifdef DPDK_NETDEV
> >>>>>> +    netdev_dpdk_set_def_lsc_int_mode_enabled(
> >>>>>> +        smap_get_bool(&ovs_cfg->other_config, "dpdk-lsc-interrupt", false));
> >>>>>> +#endif
> >>>>>> +
> >>>>> I'm still thinking that this is not necessary.
> >>>>> Eelco, why do you think we need to change the default global value in run-time?
> >>>>> IMHO, this only complicates the code. (I didn't check, but it looks like current
> >>>>> implementation will not work anyway.)
> >>>>>
> >>>> I do feel like the global option should be applied directly for the following reasons:
> >>>>
> >>>> - There is no way to see what is the running configuration
> >>>> - If not done this way the LSC mode might change for some devices, for example if you decide to change another port related
> configuration.
> >>>>
> >>> I feel that we have some kind of misunderstanding.
> >>> I'll try to explain my position:
> >>>
> >>> 1. I'm not against the global config option. I just want it to be static.
> >>>      i.e. OVS restart will be required to change it.
> >> Got your point, but why do you want it to be static? I'm arguing against it
> >> as other options, like other_config:pmd-cpu-mask, are also not static.
> >> If it can be done dynamic, I prefer doing it that way.
> > 'pmd-cpu-mask' change is the only way to configure the number of PMD threads,
> > and this is important to have this dynamic for run-time scaling. Also, this is
> > the datapath parameter, not the parameter of any ports. We, actually, have no
> > dynamic global parameters for ports. All the ports related parameters like
> > iommu-support, vhost_sock_dir are static or hardcoded.
> >
> > other_config:dpdk-lsc-interrupt is just the default value that could be
> > overridden by the dynamic per-port configuration. We're not frequently
> > adding new physical NICs to servers. At this point user likely knows, which
> > devices supports LSC interrupts and which are not, and it's possible to decide
> > which value for default other_config:dpdk-lsc-interrupt is preferable.
> >
> > I personally prefer to not have global option. Only per-port configurable
> > with 'false' as default value. This will additionally help with NICs that
> > doesn't support LSC interrupts, and with mismatch between output of ovs-vsctl
> > and real configured values. Someday in the future we could change default
> > to 'true' when most NICs will support this, and if this feature will be
> > in demand.
> Having all this complexity, I would not mind also removing the global
> setting.
> Any others in favor of keeping it? If not Robert might be able to remove
> it in V5?
> 
> >> However if we do go with a static option, it should not effect any existing
> >> NIC configuration, see 4.
> >>> 2. Per-port option will allow overriding of the global default configuration.
> >> No problem here... However it's not clear to me how we would handle PMDs not
> >> supporting RTE_ETH_DEV_INTR_LSC?For individual interfaces, we might want to
> >> check this flag, and report an error at configuration time, rather than wait
> >> for re-initialization and add something to the log file, EINVAL?
> Robert, can we add this in V5?
> >> But how do we deal with the global values and unsupported devices? Currently
> >> looking at the code it will fail initialization after the restart with no
> >> sensible error message.
> >>> 3. You're saying that the running configuration is not visible, but It'll be
> >>>      visible through global default value in other_config and per-port value in
> >>>      options of the particular interface. If we'll not set any of these configs,
> >>>      the value will not be visible in any case.
> >>>
> >>>      Suggestion: We definitely need to export the current configuration in
> >>>                  netdev_dpdk_get_config(). And it'll be always visible.
> >> I agree we need some way to show the current config because if you set the global
> >> value (and not restart) you will have no way of knowing the current operational
> >> state.The best way would be to have some ovs-appctl command to do this, as logs
> >> might rotate...
> > Current configuration could be checked by 'ovs-appctl dpif/show'. We just need
> > to add new value like 'lsc_interrupts_enabled' to netdev_dpdk_get_config().
> >
> Robert can you update this function in V5?
> 
> See also my general review of V4 before you submit a v5.
> >> Also currently all port settings are on the port, there are no global defaults.
> >> So people need to be aware that doing an "ovs-vsctl show" does not show all the
> >> actual port configuration.
> > Yes. That is not so elegant. But users should know that ovs-vsctl reports only the
> > status of the database, not the status of the daemon. 'dpif/show' and other
> > 'dp'-commands are intended to check the actual status of the working switch.
> > It's the historical behaviour of the OVS.
> >
> >>>      Maybe you're saying that we can update global default in database and this
> >>>      will hide real default value? But it's the default behaviour for many other
> >>>      'restart required' options like iommu support of vhost_sock_dir.
> >>>      We need to inform about the global default value in VLOG message while
> >>>      initializing inside dpdk_init() like it's done for all other options.
> >> Yes, and this makes it some times hard to trouble shoot.
> >>> 4. I didn't understand your second reason. There is no way to change the current
> >>>      port by changing config of the another one. DPDK initialized only once, so
> >>>      default will be fixed on boot and never changed. Any database changes for a
> >>>      global other_config after the initialization phase will be ignored.
> >> Let me explain, this is with the current implementation if we would have a global
> >> static option (i.e. v2 of this patch).Assume you configure the following:
> >>
> >> ovs-vsctl set Open_vSwitch . other_config:dpdk-lsc-interrupt=true
> >> ovs-vsctl set interface dpdk1 options:n_rxq=2
> >>
> >> The last command will trigger a re-configure of dpdk1, and causes now interrupts
> >> to be enabled.
> >>
> >> Just want to indicate its a problem with the patch as implemented, if we want to
> >> go with a static global configuration it needs to be fixed.
> > With v2 of this series first command will update only database. Configuration
> > inside daemon will not be updated. After the second command dpdk1 will be
> > reconfigured with the old value of global dpdk-lsc-interrupt, i.e. 'false'.
> > (I didn't test real v2, but it should work this way if implemented right).
> > There is no issues here. The only issue is that value in database will mismatch
> > with actual value used by daemon. After restart of ovs-vswitch, global
> > dpdk-lsc-interrupt will be updated from the database and dpdk1 will be configured
> > with enabled interrupts.
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list