* Re: [dpdk-dev] [ovs-dev] [PATCH v3 1/3] netdev-dpdk: Remove dpdk watchdog thread
[not found] ` <75A2C3A32D53AE4DB1E381A9DCA515E3371394B6@ESESSMB209.ericsson.se>
@ 2016-05-20 11:52 ` Traynor, Kevin
0 siblings, 0 replies; only message in thread
From: Traynor, Kevin @ 2016-05-20 11:52 UTC (permalink / raw)
To: Torgny Lindberg, Loftus, Ciara, dev, dev
[cross-posting to dpdk mailing list]
> -----Original Message-----
> From: Torgny Lindberg [mailto:torgny.lindberg@ericsson.com]
> Sent: Thursday, May 19, 2016 8:26 AM
> To: Traynor, Kevin <kevin.traynor@intel.com>; Loftus, Ciara
> <ciara.loftus@intel.com>; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH v3 1/3] netdev-dpdk: Remove dpdk
> watchdog thread
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Traynor,
> > Kevin
> > Sent: den 13 maj 2016 12:47
> > To: Loftus, Ciara; dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v3 1/3] netdev-dpdk: Remove dpdk
> watchdog
> > thread
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@openvswitch.org] On Behalf Of Ciara
> > > Loftus
> > > Sent: Wednesday, May 11, 2016 4:31 PM
> > > To: dev@openvswitch.org
> > > Subject: [ovs-dev] [PATCH v3 1/3] netdev-dpdk: Remove dpdk
> watchdog
> > > thread
> > >
> > > Instead of continuously polling for link status changes on 'dpdk'
> > > ports, register a callback function that will be triggered when
> DPDK
> > > detects that the link status of that port has changed.
> >
> > rte_eth_link_get_nowait() returns void, so polling it in a thread
> won't
> > indicate some kind of error in dpdk. I can't see any benefit of the
> thread
> > - using the callback means one less thread and less locking.
> >
> > Acked-by: Kevin Traynor <kevin.traynor@intel.com>
> >
>
>
> With this patch a 4s delay before detecting link-down would be
> introduced,
> which is from the viewpoint of many use cases an unacceptably long
> delay.
> I would like to suggest that the existing poll method is kept as it
> detects
> and acts on link failures much faster (millisecond time scale),
> alternatively that both poll and interrupt methods are supported
> and the one to use is selected by configuration.
>
> The delay occurs inside the dpdk driver.
> (See e.g. dpdk, ixgbe_ethdev.c, IXGBE_LINK_DOWN_CHECK_TIMEOUT)
>
Hi Torgny,
Thanks for pointing that out, I hadn't realized the additional delay.
Do you think the default should be changed in DPDK?
Kevin.
>
> Best regards,
> Torgny Lindberg
>
>
> > >
> > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
> > > Suggested-by: Kevin Traynor <kevin.traynor@intel.com>
> > > ---
> > > lib/netdev-dpdk.c | 55 ++++++++++++++++++++++++++++++------------
> ---
> > -
> > > ---------
> > > 1 file changed, 30 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > > index af86d19..89d783a 100644
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -62,8 +62,6 @@
> > > VLOG_DEFINE_THIS_MODULE(dpdk);
> > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> > >
> > > -#define DPDK_PORT_WATCHDOG_INTERVAL 5
> > > -
> > > #define OVS_CACHE_LINE_SIZE CACHE_LINE_SIZE
> > > #define OVS_VPORT_DPDK "ovs_dpdk"
> > >
> > > @@ -386,6 +384,9 @@ static int netdev_dpdk_construct(struct netdev
> *);
> > >
> > > struct virtio_net * netdev_dpdk_get_virtio(const struct
> netdev_dpdk
> > > *dev);
> > >
> > > +void link_status_changed_callback(uint8_t port_id,
> > > + enum rte_eth_event_type type OVS_UNUSED, void *param
> > > OVS_UNUSED);
> > > +
> > > static bool
> > > is_dpdk_class(const struct netdev_class *class)
> > > {
> > > @@ -536,27 +537,6 @@ check_link_status(struct netdev_dpdk *dev)
> > > }
> > > }
> > >
> > > -static void *
> > > -dpdk_watchdog(void *dummy OVS_UNUSED)
> > > -{
> > > - struct netdev_dpdk *dev;
> > > -
> > > - pthread_detach(pthread_self());
> > > -
> > > - for (;;) {
> > > - ovs_mutex_lock(&dpdk_mutex);
> > > - LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > > - ovs_mutex_lock(&dev->mutex);
> > > - check_link_status(dev);
> > > - ovs_mutex_unlock(&dev->mutex);
> > > - }
> > > - ovs_mutex_unlock(&dpdk_mutex);
> > > - xsleep(DPDK_PORT_WATCHDOG_INTERVAL);
> > > - }
> > > -
> > > - return NULL;
> > > -}
> > > -
> > > static int
> > > dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int n_rxq, int
> > > n_txq)
> > > {
> > > @@ -717,6 +697,27 @@ netdev_dpdk_alloc_txq(struct netdev_dpdk
> *dev,
> > > unsigned int n_txqs)
> > > }
> > > }
> > >
> > > +void
> > > +link_status_changed_callback(uint8_t port_id,
> > > + enum rte_eth_event_type type
> > > OVS_UNUSED,
> > > + void *param OVS_UNUSED)
> > > +{
> > > + struct netdev_dpdk *dev;
> > > +
> > > + ovs_mutex_lock(&dpdk_mutex);
> > > + LIST_FOR_EACH (dev, list_node, &dpdk_list) {
> > > + if (port_id == dev->port_id) {
> > > + ovs_mutex_lock(&dev->mutex);
> > > + check_link_status(dev);
> > > + ovs_mutex_unlock(&dev->mutex);
> > > + break;
> > > + }
> > > + }
> > > + ovs_mutex_unlock(&dpdk_mutex);
> > > +
> > > + return;
> > > +}
> > > +
> > > static int
> > > netdev_dpdk_init(struct netdev *netdev, unsigned int port_no,
> > > enum dpdk_dev_type type)
> > > @@ -774,6 +775,12 @@ netdev_dpdk_init(struct netdev *netdev,
> > unsigned
> > > int port_no,
> > > netdev_dpdk_alloc_txq(dev, OVS_VHOST_MAX_QUEUE_NUM);
> > > }
> > >
> > > + if (type == DPDK_DEV_ETH) {
> > > + rte_eth_dev_callback_register(port_no,
> > > RTE_ETH_EVENT_INTR_LSC,
> > > +
> > > (void*)link_status_changed_callback,
> > > + NULL);
> > > + }
> > > +
> > > ovs_list_push_back(&dpdk_list, &dev->list_node);
> > >
> > > unlock:
> > > @@ -3207,8 +3214,6 @@ dpdk_init__(const struct smap
> > *ovs_other_config)
> > > /* We are called from the main thread here */
> > > RTE_PER_LCORE(_lcore_id) = NON_PMD_CORE_ID;
> > >
> > > - ovs_thread_create("dpdk_watchdog", dpdk_watchdog, NULL);
> > > -
> > > #ifdef VHOST_CUSE
> > > /* Register CUSE device to handle IOCTLs.
> > > * Unless otherwise specified, cuse_dev_name is set to vhost-
> net.
> > > --
> > > 2.4.3
> > >
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > http://openvswitch.org/mailman/listinfo/dev
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply [flat|nested] only message in thread