From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0062.outbound.protection.outlook.com [104.47.37.62]) by dpdk.org (Postfix) with ESMTP id 205AB68D1 for ; Tue, 13 Sep 2016 12:57:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=harmonic.onmicrosoft.com; s=selector1-harmonicinc-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=GYCqzCgfK1z4fBw2oWjdpsgVBsmWES6WEqJtJQA8pdA=; b=q8usiaBH8Jj1joHZ/W+0sA297JxiZKTU334yA32GOiCjqzLpIvr8Mqe5pUpSXSUY2XMq/cScrGlxZ6iWbQGWFAM+e5OG1aFMKIL1R0Wa/kGKrp4xIwZVRedOUGfGrxqFA3SVOUPAoYYuhw3Kc33q5LdPrbODkmLrca8N+qw6JnQ= Received: from MWHPR11MB1360.namprd11.prod.outlook.com (10.169.235.22) by MWHPR11MB1359.namprd11.prod.outlook.com (10.169.232.22) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA_P384) id 15.1.619.10; Tue, 13 Sep 2016 10:57:17 +0000 Received: from MWHPR11MB1360.namprd11.prod.outlook.com ([10.169.235.22]) by MWHPR11MB1360.namprd11.prod.outlook.com ([10.169.235.22]) with mapi id 15.01.0609.006; Tue, 13 Sep 2016 10:57:17 +0000 From: Vladyslav Buslov To: Ferruh Yigit , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param in single threaded mode Thread-Index: AQHSDRhGYDrtizsRbEG30rZiKTDntKB3OsfQ Date: Tue, 13 Sep 2016 10:57:17 +0000 Message-ID: References: <20160906112513.26090-2-vladyslav.buslov@harmonicinc.com> <20160910135016.6468-1-vladyslav.buslov@harmonicinc.com> <20160910135016.6468-2-vladyslav.buslov@harmonicinc.com> <60f8b06a-4122-d0ef-e66c-267ee75e6c98@intel.com> In-Reply-To: <60f8b06a-4122-d0ef-e66c-267ee75e6c98@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Vladyslav.Buslov@harmonicinc.com; x-originating-ip: [95.67.66.62] x-ms-office365-filtering-correlation-id: c01ec2c6-8e5c-4582-db06-08d3dbc4ba30 x-microsoft-exchange-diagnostics: 1; MWHPR11MB1359; 20:aTuaQ/Xjyzd41/etlSnhoI6ge95frkCzXCIW1GKch+goyVt29opPyjIFwJXBv0avhF8X0ouxBwA1p97YvSNNwD2uzKLjreQABiN5Yc7AVqcD3yXhbp+NAIXWvZvhHfveB4d/MiznJmY/4DkRl9nYr3Hq17W1QbpFLbZCR9p1YLk= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:MWHPR11MB1359; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(209352067349851)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040176)(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026); SRVR:MWHPR11MB1359; BCL:0; PCL:0; RULEID:; SRVR:MWHPR11MB1359; x-forefront-prvs: 0064B3273C x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(13464003)(24454002)(199003)(377454003)(189002)(101416001)(105586002)(106356001)(99286002)(54356999)(106116001)(76176999)(50986999)(76576001)(3280700002)(2900100001)(2950100001)(77096005)(97736004)(5001770100001)(107886002)(68736007)(2906002)(305945005)(66066001)(7736002)(93886004)(7846002)(3660700001)(2501003)(74316002)(189998001)(102836003)(586003)(3846002)(6116002)(9686002)(19580405001)(19580395003)(10400500002)(5002640100001)(11100500001)(7696004)(8936002)(8676002)(81166006)(81156014)(122556002)(86362001)(87936001)(33656002)(92566002)(5660300001); DIR:OUT; SFP:1101; SCL:1; SRVR:MWHPR11MB1359; H:MWHPR11MB1360.namprd11.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; received-spf: None (protection.outlook.com: harmonicinc.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: harmonicinc.com X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Sep 2016 10:57:17.1647 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 19294cf8-3352-4dde-be9e-7f47b9b6b73d X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1359 Subject: Re: [dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param in single threaded mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Sep 2016 10:57:20 -0000 Hi Ferruh, Thanks for reviewing my code. Regarding creating kthread within locked mutex section: It is executed in c= ontext of ioctl_unlocked so I assume we may have race condition otherwise i= f multiple threads in same task try to create KNI interface simultaneously. My kernel programming knowledge is limited so correct me if I'm wrong. Regards, Vlad -----Original Message----- From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]=20 Sent: Monday, September 12, 2016 8:08 PM To: Vladyslav Buslov; dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2 2/2] kni: add support for core_id param i= n single threaded mode On 9/10/2016 2:50 PM, Vladyslav Buslov wrote: > Allow binding KNI thread to specific core in single threaded mode by=20 > setting core_id and force_bind config parameters. >=20 > Signed-off-by: Vladyslav Buslov > --- >=20 > v2: > * Fixed formatting. > * Refactored kthread create/bind functionality into separate function. > * Moved thread mode print into kni_init. > * Added short description to KNI Programmer's Gude doc. > * Fixed outdated mbuf processing description in KNI Programmer's Gude doc= . >=20 > doc/guides/prog_guide/kernel_nic_interface.rst | 5 +- > lib/librte_eal/linuxapp/kni/kni_misc.c | 72 +++++++++++++++++---= ------ > 2 files changed, 51 insertions(+), 26 deletions(-) >=20 > diff --git a/doc/guides/prog_guide/kernel_nic_interface.rst=20 > b/doc/guides/prog_guide/kernel_nic_interface.rst > index fac1960..0fdc307 100644 > --- a/doc/guides/prog_guide/kernel_nic_interface.rst > +++ b/doc/guides/prog_guide/kernel_nic_interface.rst > @@ -102,6 +102,9 @@ Refer to rte_kni_common.h in the DPDK source code for= more details. > =20 > The physical addresses will be re-mapped into the kernel address space a= nd stored in separate KNI contexts. > =20 > +The affinity of kernel RX thread (both single and multi-threaded=20 > +modes) is controlled by force_bind and core_id config parameters. > + > The KNI interfaces can be deleted by a DPDK application dynamically afte= r being created. > Furthermore, all those KNI interfaces not deleted will be deleted on=20 > the release operation of the miscellaneous device (when the DPDK applica= tion is closed). > @@ -128,7 +131,7 @@ Use Case: Ingress > On the DPDK RX side, the mbuf is allocated by the PMD in the RX thread c= ontext. > This thread will enqueue the mbuf in the rx_q FIFO. > The KNI thread will poll all KNI active devices for the rx_q. > -If an mbuf is dequeued, it will be converted to a sk_buff and sent to th= e net stack via netif_rx(). > +If an mbuf is dequeued, it will be converted to a sk_buff and sent to th= e net stack via netif_rx_ni(). Although this is small and correct modification, unrelated to this patch. I= t is good to remove from this patch, and feel free to create a separate pat= ch if you want. > The dequeued mbuf must be freed, so the same pointer is sent back in the= free_q FIFO. > =20 > The RX thread, in the same main loop, polls this FIFO and frees the mbuf= after dequeuing it. > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c=20 > b/lib/librte_eal/linuxapp/kni/kni_misc.c > index 5e7cf21..c79f5a8 100644 > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c > @@ -172,6 +172,11 @@ kni_init(void) > return -EINVAL; > } > =20 > + if (multiple_kthread_on =3D=3D 0) > + KNI_PRINT("Single kernel thread for all KNI devices\n"); > + else > + KNI_PRINT("Multiple kernel thread mode enabled\n"); > + Instead of fixing these in a second patch, why not do the correct one in fi= rst patch? Or I think it is better to have a single patch instead of two. W= hat about squashing second patch into first? > #ifdef HAVE_SIMPLIFIED_PERNET_OPERATIONS > rc =3D register_pernet_subsys(&kni_net_ops); > #else > @@ -240,12 +245,6 @@ kni_open(struct inode *inode, struct file *file) > if (test_and_set_bit(KNI_DEV_IN_USE_BIT_NUM, &knet->device_in_use)) > return -EBUSY; > =20 > - /* Create kernel thread for single mode */ > - if (multiple_kthread_on =3D=3D 0) > - KNI_PRINT("Single kernel thread for all KNI devices\n"); > - else > - KNI_PRINT("Multiple kernel thread mode enabled\n"); > - > file->private_data =3D get_net(net); > KNI_PRINT("/dev/kni opened\n"); > =20 > @@ -391,6 +390,32 @@ kni_check_param(struct kni_dev *kni, struct rte_kni_= device_info *dev) > return 0; > } > =20 > +__printf(5, 6) static struct task_struct * kni_run_thread(int=20 > +(*threadfn)(void *data), > + void *data, > + uint8_t force_bind, > + unsigned core_id, > + const char namefmt[], ...) Syntax should be updated. > +{ > + struct task_struct *kni_thread =3D NULL; > + char task_comm[TASK_COMM_LEN]; > + va_list args; > + > + va_start(args, namefmt); > + vsnprintf(task_comm, sizeof(task_comm), namefmt, args); > + va_end(args); What about just using a "char *" and make things simpler, instead of variab= le length parameters. Name can be kni_%s, for multi_thread %s is kni->name, for single_thread %s is "single". > + > + kni_thread =3D kthread_create(threadfn, data, task_comm); > + if (IS_ERR(kni_thread)) > + return NULL; > + > + if (force_bind) > + kthread_bind(kni_thread, core_id); > + wake_up_process(kni_thread); > + > + return kni_thread; > +} > + > static int > kni_ioctl_create(struct net *net, > unsigned int ioctl_num, unsigned long ioctl_param) @@ -419,8 +444,7=20 > @@ kni_ioctl_create(struct net *net, > /** > * Check if the cpu core id is valid for binding. > */ > - if (dev_info.force_bind && > - !cpu_online(dev_info.core_id)) { > + if (dev_info.force_bind && !cpu_online(dev_info.core_id)) { Same comment as above, lets have a single patch. > KNI_ERR("cpu %u is not online\n", dev_info.core_id); > return -EINVAL; > } > @@ -572,31 +596,29 @@ kni_ioctl_create(struct net *net, > * and finally wake it up. > */ > if (multiple_kthread_on) { > - kni->pthread =3D kthread_create(kni_thread_multiple, > - (void *)kni, > - "kni_%s", kni->name); > - if (IS_ERR(kni->pthread)) { > + kni->pthread =3D kni_run_thread(kni_thread_multiple, > + (void *)kni, > + dev_info.force_bind, > + kni->core_id, > + "kni_%s", kni->name); What about passing "kni" as parameter, this saves force_bind and core_id values: static struct task_struct * kni_run_thread(int (*threadfn)(void *data), void *data, struct kni_dev *kni= , char *thread_name); > + if (kni->pthread =3D=3D NULL) { > kni_dev_remove(kni); > return -ECANCELED; > } > - if (dev_info.force_bind) > - kthread_bind(kni->pthread, kni->core_id); > - wake_up_process(kni->pthread); > } else { > mutex_lock(&knet->kni_kthread_lock); > if (knet->kni_kthread =3D=3D NULL) { > - knet->kni_kthread =3D kthread_create(kni_thread_single, > - (void *)knet, > - "kni_single"); > - if (IS_ERR(knet->kni_kthread)) { > - kni_dev_remove(kni); > - return -ECANCELED; > - } > - if (dev_info.force_bind) > - kthread_bind(knet->kni_kthread, kni->core_id); > - wake_up_process(knet->kni_kthread); > + knet->kni_kthread =3D kni_run_thread(kni_thread_single, > + (void *)knet, > + dev_info.force_bind, > + kni->core_id, > + "kni_single"); Syntax should be updated, not need to have a new line per param. > } > mutex_unlock(&knet->kni_kthread_lock); > + if (knet->kni_kthread =3D=3D NULL) { Does this needs to be within kthread_lock? > + kni_dev_remove(kni); > + return -ECANCELED; > + } > } > =20 > down_write(&knet->kni_list_lock); >=20