From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 947E6A0C41; Fri, 16 Apr 2021 10:19:07 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 832CF141BA9; Fri, 16 Apr 2021 10:19:07 +0200 (CEST) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 0743A141BA9 for ; Fri, 16 Apr 2021 10:19:06 +0200 (CEST) Received: from DGGEMS401-HUB.china.huawei.com (unknown [172.30.72.59]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4FM8DT1CQ8zPqpF; Fri, 16 Apr 2021 16:16:09 +0800 (CST) Received: from [10.67.103.128] (10.67.103.128) by DGGEMS401-HUB.china.huawei.com (10.3.19.201) with Microsoft SMTP Server id 14.3.498.0; Fri, 16 Apr 2021 16:19:00 +0800 To: "Power, Ciara" , "dev@dpdk.org" CC: "Yigit, Ferruh" , "Pattan, Reshma" , "david.marchand@redhat.com" References: <1618051453-21264-1-git-send-email-humin29@huawei.com> <1618487412-26678-1-git-send-email-humin29@huawei.com> <1618487412-26678-2-git-send-email-humin29@huawei.com> From: "Min Hu (Connor)" Message-ID: Date: Fri, 16 Apr 2021 16:19:00 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.3.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="gbk"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.128] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v3 1/2] telemetry: fix missing check for thread creation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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" Thanks Power, v4 has been sent, please check it out. ÔÚ 2021/4/16 0:11, Power, Ciara дµÀ: > Hi Connor, > > Thanks for looking at this. Left some comments inline. > > Ciara > >> -----Original Message----- >> From: Min Hu (Connor) >> Sent: Thursday 15 April 2021 12:50 >> To: dev@dpdk.org >> Cc: Yigit, Ferruh ; Power, Ciara >> ; Pattan, Reshma ; >> david.marchand@redhat.com >> Subject: [PATCH v3 1/2] telemetry: fix missing check for thread creation >> >> From: Chengwen Feng >> >> Add result check and message print out for thread creation after failure. >> >> Fixes: b80fe1805eee ("telemetry: introduce backward compatibility") >> Cc: stable@dpdk.org >> >> Signed-off-by: Chengwen Feng >> Signed-off-by: Min Hu (Connor) >> --- >> lib/librte_telemetry/telemetry.c | 30 +++++++++++++++++++++++++++- >> -- >> lib/librte_telemetry/telemetry_legacy.c | 10 ++++++++-- >> 2 files changed, 35 insertions(+), 5 deletions(-) >> >> diff --git a/lib/librte_telemetry/telemetry.c >> b/lib/librte_telemetry/telemetry.c >> index 7e08afd..e6a99f3 100644 >> --- a/lib/librte_telemetry/telemetry.c >> +++ b/lib/librte_telemetry/telemetry.c >> @@ -350,6 +350,7 @@ socket_listener(void *socket) { >> while (1) { >> pthread_t th; >> + int rc; >> struct socket *s = (struct socket *)socket; >> int s_accepted = accept(s->sock, NULL, NULL); >> if (s_accepted < 0) { >> @@ -366,7 +367,12 @@ socket_listener(void *socket) >> __atomic_add_fetch(s->num_clients, 1, >> __ATOMIC_RELAXED); >> } >> - pthread_create(&th, NULL, s->fn, (void >> *)(uintptr_t)s_accepted); >> + rc = pthread_create(&th, NULL, s->fn, (void >> *)(uintptr_t)s_accepted); >> + if (rc != 0) { >> + TMTY_LOG(ERR, "Error with create client thread\n"); >> + close(s_accepted); >> + return NULL; > > > I think this should be "continue" instead of "return NULL" - if one client connection thread fails, > we probably want to keep listening for more clients rather than stop completely. > > The "s->num_clients" counter should be decremented here too, > it gets incremented above when the connection is accepted, but if we are closing the connection, > the counter should reflect that. > > >> + } >> pthread_detach(th); >> } >> return NULL; >> @@ -425,6 +431,7 @@ static int >> telemetry_legacy_init(void) >> { >> pthread_t t_old; >> + int rc; >> >> if (num_legacy_callbacks == 1) { >> TMTY_LOG(WARNING, "No legacy callbacks, legacy socket not >> created\n"); @@ -440,7 +447,15 @@ telemetry_legacy_init(void) >> v1_socket.sock = create_socket(v1_socket.path); >> if (v1_socket.sock < 0) >> return -1; >> - pthread_create(&t_old, NULL, socket_listener, &v1_socket); >> + rc = pthread_create(&t_old, NULL, socket_listener, &v1_socket); >> + if (rc != 0) { >> + TMTY_LOG(ERR, "Error with create thread for legacy >> socket\n"); >> + close(v1_socket.sock); >> + v1_socket.sock = -1; >> + unlink(v1_socket.path); >> + v1_socket.path[0] = '\0'; >> + return -1; >> + } >> pthread_setaffinity_np(t_old, sizeof(*thread_cpuset), >> thread_cpuset); >> >> TMTY_LOG(DEBUG, "Legacy telemetry socket initialized ok\n"); @@ - >> 451,6 +466,7 @@ static int >> telemetry_v2_init(void) >> { >> pthread_t t_new; >> + int rc; >> >> v2_socket.num_clients = &v2_clients; >> rte_telemetry_register_cmd("/", list_commands, @@ -469,7 +485,15 >> @@ telemetry_v2_init(void) >> v2_socket.sock = create_socket(v2_socket.path); >> if (v2_socket.sock < 0) >> return -1; >> - pthread_create(&t_new, NULL, socket_listener, &v2_socket); >> + rc = pthread_create(&t_new, NULL, socket_listener, &v2_socket); >> + if (rc != 0) { >> + TMTY_LOG(ERR, "Error with create thread for socket"); >> + close(v2_socket.sock); >> + v2_socket.sock = -1; >> + unlink(v2_socket.path); >> + v2_socket.path[0] = '\0'; >> + return -1; >> + } >> pthread_setaffinity_np(t_new, sizeof(*thread_cpuset), >> thread_cpuset); >> atexit(unlink_sockets); >> >> diff --git a/lib/librte_telemetry/telemetry_legacy.c >> b/lib/librte_telemetry/telemetry_legacy.c >> index 5e9af37..fd242bf 100644 >> --- a/lib/librte_telemetry/telemetry_legacy.c >> +++ b/lib/librte_telemetry/telemetry_legacy.c >> @@ -83,6 +83,7 @@ register_client(const char *cmd __rte_unused, const >> char *params, >> pthread_t th; >> char data[BUF_SIZE]; >> int fd; >> + int rc; >> struct sockaddr_un addrs; >> #endif /* !RTE_EXEC_ENV_WINDOWS */ >> >> @@ -112,8 +113,13 @@ register_client(const char *cmd __rte_unused, const >> char *params, >> close(fd); >> return -1; >> } >> - pthread_create(&th, NULL, &legacy_client_handler, >> - (void *)(uintptr_t)fd); >> + rc = pthread_create(&th, NULL, &legacy_client_handler, >> + (void *)(uintptr_t)fd); >> + if (rc != 0) { >> + perror("Failed to create legacy client thread\n"); >> + close(fd); >> + return -1; >> + } >> #endif /* !RTE_EXEC_ENV_WINDOWS */ >> return 0; >> } >> -- >> 2.7.4 > > . >