Hi Stephen,
Thank you for your valuable feedback.
I will fix it in next v11 patch series.
Thank you.
------------------------------------------------------------------
发件人：Stephen Hemminger <stephen@networkplumber.org>
发送时间：2025年9月24日(周三) 23:27
收件人：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 Wed, 24 Sep 2025 17:40:00 +0800
"Dimon" <dimon.zhao@nebula-matrix.com> wrote:
> 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?
> 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.
Sure a bit or flag is a good alternate solution.
You do want to still close the pipe in the main thread
to cause the read() to get unblocked.