From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E992EA04DD; Thu, 2 Jan 2020 10:58:08 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 7F71D1BFFB; Thu, 2 Jan 2020 10:58:08 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id A19751BF85; Thu, 2 Jan 2020 10:58:05 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Jan 2020 01:58:04 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,386,1571727600"; d="scan'208";a="244569348" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga004.fm.intel.com with ESMTP; 02 Jan 2020 01:58:04 -0800 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 2 Jan 2020 01:58:04 -0800 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 2 Jan 2020 01:58:03 -0800 Received: from shsmsx106.ccr.corp.intel.com ([169.254.10.139]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.39]) with mapi id 14.03.0439.000; Thu, 2 Jan 2020 17:58:01 +0800 From: "Zhu, TaoX" To: "Ananyev, Konstantin" CC: "dev@dpdk.org" , "stable@dpdk.org" , "Lu, Wenzhuo" , "Zhang, Qi Z" , "Xing, Beilei" , "Yu, PingX" Thread-Topic: [PATCH] net/ixgbe: fix blocking system events Thread-Index: AQHVu5berdftKOHXk0mu/fDo/Ghhe6fSNSIAgATqLkA= Date: Thu, 2 Jan 2020 09:58:01 +0000 Message-ID: <60652C6914E08D41B9AA1580751B3CA9C24A10@SHSMSX106.ccr.corp.intel.com> References: <1577328342-216505-1-git-send-email-taox.zhu@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix blocking system events X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, Thank you for your advice. Your advice is excellent. I have the same idea w= ith you to solve this problem. First. Consider whether the fallback function of alamer can shorten the ti= me.=20 But it seems difficult for the following reasons(If you have a good idea, p= lease let me know): a. The entry of the fallback function is 'ixgbe_setup_mac_link_multispeed_= fiber', It's a little more complicated b. This callback function not only has multiple sleeps, but also may have = long sleep in other functions it calls c. The function called by the callback function has different implementati= ons for different NIC d. This callback function and other functions it calls are all from ND's s= hared code. After modification, upgrading will cause trouble So I use a separate thread to replace alarm, This thread is created using w= rapper function: rte_ctrl_thread_create(). A mechanism is provided to create a thread when PF or VF is initialized for= the first time. This thread will select tasks from the task list in order = to execute. If the task list is empty, the thread sleeps.=20 Provides interface for adding and deleting tasks. The specific implementation is as follows(See patch for detailed implementa= tion): a. Thread create function: ixgbe_task_thread_init, it will call rte_ctrl_t= hread_create to create thread b. Thread destroy function: ixgbe_task_thread_uninit c. Add task function: ixgbe_add_task, it add a task and wake up the sleepi= ng thread d. Cancel task function: ixgbe_cancel_task, it cancel a task synchronously The reason why a task queue is used to manage tasks is that there may be co= ncurrent conflicts, because alarm has no concurrent conflicts. BR, Zhu, Tao > -----Original Message----- > From: Ananyev, Konstantin > Sent: Monday, December 30, 2019 9:58 PM > To: Zhu, TaoX ; konstantin.ananyev@intel.com > wenzhuo.lu@intel.com > Cc: dev@dpdk.org; stable@dpdk.org > Subject: RE: [PATCH] net/ixgbe: fix blocking system events >=20 > Hi, >=20 > > IXGBE link status task use rte alarm thread in old implementation. > > Sometime ixgbe link status task takes up to 9 seconds. This will > > severely affect the rte-alarm-thread-dependent a task in the system, > > like interrupt or hotplug event. So replace with a independent thread > > which has the same thread affinity settings as rte interrupt. >=20 >=20 > I don't think it is a good idea to add into PMD code that creates/manages > new threads. > I think ideal would be to rework link setup code, so it can take less tim= e > (make it interruptable/repeatable). > If that is not possible, and control function is the only way, there is a= wrapper > function: rte_ctrl_thread_create() that can be used here instead of creat= ing > your own framework. >=20 >=20 > > > > Fixes: 0408f47b ("net/ixgbe: fix busy polling while fiber link > > update") > > Cc: stable@dpdk.org > > > > Signed-off-by: Zhu Tao > > --- > > drivers/net/ixgbe/ixgbe_ethdev.c | 184 > > +++++++++++++++++++++++++++++++++++++-- > > drivers/net/ixgbe/ixgbe_ethdev.h | 32 +++++++ > > 2 files changed, 210 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > > b/drivers/net/ixgbe/ixgbe_ethdev.c > > index 2c6fd0f..f0b387d 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > @@ -15,6 +15,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -378,6 +379,9 @@ static int ixgbe_dev_udp_tunnel_port_del(struct > rte_eth_dev *dev, > > struct rte_eth_udp_tunnel *udp_tunnel); static int > > ixgbe_filter_restore(struct rte_eth_dev *dev); static void > > ixgbe_l2_tunnel_conf(struct rte_eth_dev *dev); > > +static int ixgbe_task_thread_init(struct rte_eth_dev *dev); static > > +void ixgbe_task_thread_uninit(struct rte_eth_dev *dev); > > + > > > > /* > > * Define VF Stats MACRO for Non "cleared on read" register @@ > > -1069,6 +1073,171 @@ struct rte_ixgbe_xstats_name_off { } > > > > /* > > + * Add a task to task queue tail. > > + */ > > +int ixgbe_add_task(struct rte_eth_dev *dev, ixgbe_task_cb_fn task_cb) > > +{ struct ixgbe_adapter *ad =3D dev->data->dev_private; struct > > +ixgbe_task *task; > > + > > +if (ad->task_status =3D=3D IXGBE_TASK_THREAD_RUNNING) { task =3D > > +rte_zmalloc("ixgbe", sizeof(struct ixgbe_task), 0); if (task =3D=3D NU= LL) > > +return -ENOMEM; > > + > > +task->arg =3D dev; > > +task->task_cb =3D task_cb; > > +task->status =3D IXGBE_TASK_READY; > > + > > +pthread_mutex_lock(&ad->task_lock); > > +TAILQ_INSERT_TAIL(&ad->task_head, task, next); > > +pthread_cond_signal(&ad->task_cond); > > +pthread_mutex_unlock(&ad->task_lock); > > +} else { > > +return -EPERM;/* Operation not permitted */ } > > + > > +return 0; > > +} > > + > > +/* > > + * Sync cancel a task with all @task_cb be exit. > > + */ > > +int ixgbe_cancel_task(struct rte_eth_dev *dev, ixgbe_task_cb_fn > > +task_cb) { struct ixgbe_adapter *ad =3D dev->data->dev_private; struct > > +ixgbe_task *task, *ttask; int i, executing; > > +#define DELAY_TIMEOUT_LOG 2000// 2s > > +#define DELAY_TIMEOUT_MAX 10000// 10s > > + > > +for (i =3D 0; i < DELAY_TIMEOUT_MAX; i++) { executing =3D 0; if > > +(ad->task_status =3D=3D IXGBE_TASK_THREAD_RUNNING) { > > +pthread_mutex_lock(&ad->task_lock); > > +TAILQ_FOREACH_SAFE(task, &ad->task_head, next, ttask) { if > > +(task->task_cb =3D=3D task_cb) { if (task->status =3D=3D IXGBE_TASK_RU= NNING) > > +{ > > +executing++; > > +} else { > > +TAILQ_REMOVE(&ad->task_head, task, next); rte_free(task); } } } > > +pthread_mutex_unlock(&ad->task_lock); > > + > > +if (executing) { > > +if (i > DELAY_TIMEOUT_LOG && (i % 1000 =3D=3D 0)) > { PMD_DRV_LOG(WARNING, > > + "Cannel task time wait %ds!", i / 1000); } > > + > > +rte_delay_us_sleep(1000);// 1ms > > +continue; > > +} > > +} > > +break; > > +} > > + > > +if (i =3D=3D DELAY_TIMEOUT_MAX) > > +return -EBUSY; > > + > > +return 0; > > +} > > + > > +/* > > + * Task main thread. Loop until state is set to IXGBE_TASK_THREAD_EXIT= . > > + * For each task, set the status to IXGBE_TASK_RUNNING before > > +execution, > > + * execute and then be dequeue. > > + */ > > +static void *ixgbe_task_handler(void *args) { struct ixgbe_adapter > > +*ad =3D > > + ((struct rte_eth_dev *)args)->data->dev_private; struct > > +ixgbe_task *task; > > + > > +PMD_INIT_LOG(DEBUG, "ixgbe task thread created"); while > > +(ad->task_status) { pthread_mutex_lock(&ad->task_lock); > > +if (TAILQ_EMPTY(&ad->task_head)) { > > +pthread_cond_wait(&ad->task_cond, &ad->task_lock); > > +pthread_mutex_unlock(&ad->task_lock); > > +continue; > > +} > > + > > +/* pop firt task and run it */ > > +task =3D TAILQ_FIRST(&ad->task_head); > > +task->status =3D IXGBE_TASK_RUNNING; > > +pthread_mutex_unlock(&ad->task_lock); > > + > > +if (task && task->task_cb) > > +task->task_cb(task->arg); > > + > > +pthread_mutex_lock(&ad->task_lock); > > +TAILQ_REMOVE(&ad->task_head, task, next); rte_free(task); > > +pthread_mutex_unlock(&ad->task_lock); > > +} > > + > > +pthread_mutex_lock(&ad->task_lock); > > +while (!TAILQ_EMPTY(&ad->task_head)) { task =3D > > +TAILQ_FIRST(&ad->task_head); TAILQ_REMOVE(&ad->task_head, task, > > +next); rte_free(task); } pthread_mutex_unlock(&ad->task_lock); > > +ad->task_status =3D IXGBE_TASK_THREAD_EXIT_DONE; > > + > > +return NULL; > > +} > > + > > +static int ixgbe_task_thread_init(struct rte_eth_dev *dev) { struct > > +ixgbe_adapter *ad =3D dev->data->dev_private; struct rte_pci_device > > +*pci_dev =3D RTE_ETH_DEV_TO_PCI(dev); char > > +name[IXGBE_TASK_THREAD_NAME_LEN]; > > +int ret; > > + > > +TAILQ_INIT(&ad->task_head); > > +pthread_mutex_init(&ad->task_lock, NULL); > > +pthread_cond_init(&ad->task_cond, NULL); > > + > > +snprintf(name, IXGBE_TASK_THREAD_NAME_LEN, "ixgbe-delay:%s", > > +pci_dev->name); > > + > > +ret =3D rte_ctrl_thread_create(&ad->task_tid, name, NULL, > > + ixgbe_task_handler, dev); > > +if (ret < 0) { > > +PMD_INIT_LOG(ERR, "Create control thread %s fail!", name); return > > +ret; } > > +ad->task_status =3D IXGBE_TASK_THREAD_RUNNING; > > + > > +return 0; > > +} > > + > > +static void ixgbe_task_thread_uninit(struct rte_eth_dev *dev) { > > +struct ixgbe_adapter *ad =3D dev->data->dev_private; int i; #define > > +MAX_EXIT_TIMEOUT 3000/* 3s */ > > + > > +if (ad->task_status =3D=3D IXGBE_TASK_THREAD_RUNNING) { > > +ad->task_status =3D IXGBE_TASK_THREAD_EXIT; > > +pthread_cond_signal(&ad->task_cond); > > +for (i =3D 0; i < MAX_EXIT_TIMEOUT; i++) { if (ad->task_status =3D=3D > > +IXGBE_TASK_THREAD_EXIT_DONE) { pthread_cond_destroy(&ad- > >task_cond); > > +pthread_mutex_destroy(&ad->task_lock); > > +break; > > +} > > +rte_delay_us_sleep(1000);// 1ms > > +} > > +} > > +} > > + > > +/* > > * This function is based on code in ixgbe_attach() in base/ixgbe.c. > > * It returns 0 on success. > > */ > > @@ -1301,6 +1470,7 @@ struct rte_ixgbe_xstats_name_off { > > /* enable support intr */ > > ixgbe_enable_intr(eth_dev); > > > > +ixgbe_task_thread_init(eth_dev); > > ixgbe_dev_set_link_down(eth_dev); > > > > /* initialize filter info */ > > @@ -1337,6 +1507,7 @@ struct rte_ixgbe_xstats_name_off { return 0; > > > > ixgbe_dev_close(eth_dev); > > +ixgbe_task_thread_uninit(eth_dev); > > > > return 0; > > } > > @@ -1713,6 +1884,7 @@ static int ixgbe_l2_tn_filter_init(struct > rte_eth_dev *eth_dev) > > ixgbevf_dev_interrupt_handler, eth_dev); > > rte_intr_enable(intr_handle); ixgbevf_intr_enable(eth_dev); > > +ixgbe_task_thread_init(eth_dev); > > > > PMD_INIT_LOG(DEBUG, "port %d vendorID=3D0x%x deviceID=3D0x%x > mac.type=3D%s", > > eth_dev->data->port_id, pci_dev->id.vendor_id, @@ -1732,6 > > +1904,7 @@ static int ixgbe_l2_tn_filter_init(struct rte_eth_dev > > *eth_dev) return 0; > > > > ixgbevf_dev_close(eth_dev); > > +ixgbe_task_thread_uninit(eth_dev); > > > > return 0; > > } > > @@ -2570,7 +2743,7 @@ static int eth_ixgbevf_pci_remove(struct > > rte_pci_device *pci_dev) } > > > > /* Stop the link setup handler before resetting the HW. */ > > -rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev); > > +ixgbe_cancel_task(dev, ixgbe_dev_setup_link_alarm_handler); > > > > /* disable uio/vfio intr/eventfd mapping */ > > rte_intr_disable(intr_handle); @@ -2842,7 +3015,7 @@ static int > > eth_ixgbevf_pci_remove(struct rte_pci_device *pci_dev) > > > > PMD_INIT_FUNC_TRACE(); > > > > -rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev); > > +ixgbe_cancel_task(dev, ixgbe_dev_setup_link_alarm_handler); > > > > /* disable interrupts */ > > ixgbe_disable_intr(hw); > > @@ -4162,8 +4335,7 @@ static int > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev, > if > > (link_up =3D=3D 0) { if (ixgbe_get_media_type(hw) =3D=3D > > ixgbe_media_type_fiber) { intr->flags |=3D > IXGBE_FLAG_NEED_LINK_CONFIG; > > -rte_eal_alarm_set(10, -ixgbe_dev_setup_link_alarm_handler, dev); > > +ixgbe_add_task(dev, ixgbe_dev_setup_link_alarm_handler); > > } > > return rte_eth_linkstatus_set(dev, &link); } @@ -5207,7 +5379,7 @@ > > static int ixgbevf_dev_xstats_get_names(__rte_unused struct > > rte_eth_dev *dev, PMD_INIT_FUNC_TRACE(); > > > > /* Stop the link setup handler before resetting the HW. */ > > -rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev); > > +ixgbe_cancel_task(dev, ixgbe_dev_setup_link_alarm_handler); > > > > err =3D hw->mac.ops.reset_hw(hw); > > if (err) { > > @@ -5305,7 +5477,7 @@ static int > > ixgbevf_dev_xstats_get_names(__rte_unused struct rte_eth_dev *dev, > > > > PMD_INIT_FUNC_TRACE(); > > > > -rte_eal_alarm_cancel(ixgbe_dev_setup_link_alarm_handler, dev); > > +ixgbe_cancel_task(dev, ixgbe_dev_setup_link_alarm_handler); > > > > ixgbevf_intr_disable(dev); > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.h > > b/drivers/net/ixgbe/ixgbe_ethdev.h > > index 76a1b9d..4426349 100644 > > --- a/drivers/net/ixgbe/ixgbe_ethdev.h > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.h > > @@ -470,6 +470,28 @@ struct ixgbe_tm_conf { bool committed; }; > > > > +#define IXGBE_TASK_THREAD_NAME_LEN 64 enum > { IXGBE_TASK_THREAD_EXIT > > +=3D 0, IXGBE_TASK_THREAD_RUNNING =3D 1, > IXGBE_TASK_THREAD_EXIT_DONE =3D 2 > > +}; enum { IXGBE_TASK_READY =3D 0, IXGBE_TASK_RUNNING =3D 1, > > +IXGBE_TASK_FINISH =3D 2 }; > > + > > +typedef void (*ixgbe_task_cb_fn) (void *arg); struct ixgbe_task { > > +TAILQ_ENTRY(ixgbe_task) next; > > +void *arg; > > +int status; > > +ixgbe_task_cb_fn task_cb; > > +}; > > +TAILQ_HEAD(ixgbe_task_head, ixgbe_task); > > + > > + > > /* > > * Structure to store private data for each driver instance (for each = port). > > */ > > @@ -510,6 +532,13 @@ struct ixgbe_adapter { > > * mailbox status) link status. > > */ > > uint8_t pflink_fullchk; > > + > > +/* Control thread per VF/PF, used for to do delay work */ pthread_t > > +task_tid; struct ixgbe_task_head task_head; int task_status; > > +pthread_mutex_t task_lock; pthread_cond_t task_cond; > > }; > > > > struct ixgbe_vf_representor { > > @@ -760,6 +789,9 @@ void ixgbe_dev_macsec_setting_save(struct > > rte_eth_dev *dev, struct ixgbe_macsec_setting *macsec_setting); > > > > void ixgbe_dev_macsec_setting_reset(struct rte_eth_dev *dev); > > +int ixgbe_add_task(struct rte_eth_dev *dev, ixgbe_task_cb_fn > > +task_cb); int ixgbe_cancel_task(struct rte_eth_dev *dev, > > +ixgbe_task_cb_fn task_cb); > > + > > > > static inline int > > ixgbe_ethertype_filter_lookup(struct ixgbe_filter_info *filter_info, > > -- > > 1.8.3.1 >=20