From: "Traynor, Kevin" <kevin.traynor@intel.com>
To: Torgny Lindberg <torgny.lindberg@ericsson.com>,
"Loftus, Ciara" <ciara.loftus@intel.com>,
"dev@openvswitch.org" <dev@openvswitch.org>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [ovs-dev] [PATCH v3 1/3] netdev-dpdk: Remove dpdk watchdog thread
Date: Fri, 20 May 2016 11:52:56 +0000 [thread overview]
Message-ID: <BC0FEEC7D7650749874CEC11314A88F74558E47F@IRSMSX104.ger.corp.intel.com> (raw)
In-Reply-To: <75A2C3A32D53AE4DB1E381A9DCA515E3371394B6@ESESSMB209.ericsson.se>
[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
parent reply other threads:[~2016-05-20 11:53 UTC|newest]
Thread overview: expand[flat|nested] mbox.gz Atom feed
[parent not found: <75A2C3A32D53AE4DB1E381A9DCA515E3371394B6@ESESSMB209.ericsson.se>]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BC0FEEC7D7650749874CEC11314A88F74558E47F@IRSMSX104.ger.corp.intel.com \
--to=kevin.traynor@intel.com \
--cc=ciara.loftus@intel.com \
--cc=dev@dpdk.org \
--cc=dev@openvswitch.org \
--cc=torgny.lindberg@ericsson.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).