Hi Stephen,
Thank you for your valuable feedback. I fully understand your concern about pthread_cancel.
However, due to our chip design constraints, during the remove process, a few mailbox messages will still be sent after disabling mailbox interrupts.
Since mailbox interrupts are already closed at this point, we cannot rely on pipe reads to trigger mailbox ACK reception.
Therefore, the acks of these mailbox messages have to be received using polling. This is why we need the NBL_CHAN_INTERRUPT_READY flag.
I've implemented an alternative, adding a state flag NBL_CHAN_TEARDOWN to notify the polling task to exit.
Here's the code modification.
I've tested this on our hardware and it works fine.
Do you think this modification is okay?
diff --git a/drivers/net/nbl/nbl_hw/nbl_channel.c b/drivers/net/nbl/nbl_hw/nbl_channel.c
index a68060799c..c8572b3b4a 100644
--- a/drivers/net/nbl/nbl_hw/nbl_channel.c
+++ b/drivers/net/nbl/nbl_hw/nbl_channel.c
 static uint32_t nbl_chan_thread_polling_task(void *param)
 {
 struct nbl_channel_mgt *chan_mgt = (struct nbl_channel_mgt *)param;
 union nbl_chan_info *chan_info = NBL_CHAN_MGT_TO_CHAN_INFO(chan_mgt);
 struct timespec time;
 char unused[16];
 ssize_t nr = 0;
 time.tv_sec = 0;
 time.tv_nsec = 100000;
 while (true) {
 if (rte_bitmap_get(chan_info->mailbox.state_bmp, NBL_CHAN_INTERRUPT_READY)) {
 nr = read(chan_info->mailbox.fd[0], &unused, sizeof(unused));
 if (nr < 0)
 break;
- }
+ } else if (rte_bitmap_get(chan_info->mailbox.state_bmp, NBL_CHAN_TEARDOWN)) {
+ break;
+ }
 nbl_chan_clean_queue(chan_mgt);
 nanosleep(&time, 0);
 }
 return 0;
 }
 static int nbl_chan_task_finish(void *priv)
 {
 struct nbl_channel_mgt *chan_mgt = (struct nbl_channel_mgt *)priv;
 union nbl_chan_info *chan_info = NBL_CHAN_MGT_TO_CHAN_INFO(chan_mgt);
- pthread_cancel((pthread_t)chan_info->mailbox.tid.opaque_id);
+ /* set teardown state to cause mailbox thread to exit */
+ nbl_chan_set_queue_state(priv, NBL_CHAN_TEARDOWN, true);
 close(chan_info->mailbox.fd[0]);
 close(chan_info->mailbox.fd[1]);
 chan_info->mailbox.fd[0] = -1;
 chan_info->mailbox.fd[1] = -1;
 rte_thread_join(chan_info->mailbox.tid, NULL);
 return 0;
 }
------------------------------------------------------------------
发件人：Stephen Hemminger <stephen@networkplumber.org>
发送时间：2025年9月24日(周三) 02:25
收件人：Dimon<dimon.zhao@nebula-matrix.com>
抄　送：dev<dev@dpdk.org>; Kyo Liu<kyo.liu@nebula-matrix.com>; Leon<leon.yu@nebula-matrix.com>; Sam<sam.chen@nebula-matrix.com>
主　题：Re: [PATCH v10 04/17] net/nbl: add Channel layer definitions and implementation
On Mon, 22 Sep 2025 20:53:49 -0700
Dimon Zhao <dimon.zhao@nebula-matrix.com> wrote:
> +
> +static int nbl_chan_task_finish(void *priv)
> +{
> + struct nbl_channel_mgt *chan_mgt = (struct nbl_channel_mgt *)priv;
> + union nbl_chan_info *chan_info = NBL_CHAN_MGT_TO_CHAN_INFO(chan_mgt);
> +
> + pthread_cancel((pthread_t)chan_info->mailbox.tid.opaque_id);
> + close(chan_info->mailbox.fd[0]);
> + close(chan_info->mailbox.fd[1]);
> + chan_info->mailbox.fd[0] = -1;
> + chan_info->mailbox.fd[1] = -1;
> + rte_thread_join(chan_info->mailbox.tid, NULL);
> + return 0;
> +}
Using pthread_cancel has a couple of problems: pthread_cancel uses a signal and there is no safe equivalent on Windows
A better alternative is to just close the pipe and then the read
will safely exit the read(). Something like this:
diff --git a/drivers/net/nbl/nbl_hw/nbl_channel.c b/drivers/net/nbl/nbl_hw/nbl_channel.c
index a68060799c..c8572b3b4a 100644
--- a/drivers/net/nbl/nbl_hw/nbl_channel.c
+++ b/drivers/net/nbl/nbl_hw/nbl_channel.c
@@ -551,7 +551,7 @@ static int nbl_chan_task_finish(void *priv)
 struct nbl_channel_mgt *chan_mgt = (struct nbl_channel_mgt *)priv;
 union nbl_chan_info *chan_info = NBL_CHAN_MGT_TO_CHAN_INFO(chan_mgt);
- pthread_cancel((pthread_t)chan_info->mailbox.tid.opaque_id);
+ /* closing pipe will cause mailbox thread to exit */
 close(chan_info->mailbox.fd[0]);
 close(chan_info->mailbox.fd[1]);
 chan_info->mailbox.fd[0] = -1;
Obviously, I don't have hardware so not tested.
Yes, other drivers call pthread_cancel() and I am working on an RFC
patch series to eliminate all those places.