RE: [PATCH v5] net/ice: fix ice dcf control thread crash

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Tuesday, March 21, 2023 10:08 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou, YidingX
> <yidingx.zhou@intel.com>; Zhang, Ke1X <ke1x.zhang@intel.com>
> Subject: RE: [PATCH v5] net/ice: fix ice dcf control thread crash
> 
> Hi Qi, here is my new solution, can you give me some good suggestions.
> 1. remove the 'vc_event_msg_cb == NULL' related processing and let each
> 'ice-rest' thread, end normally.
> 2. Define vsi_update_thread_num as rte_atomic32_t.
> 
> > -----Original Message-----
> > From: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Sent: 2023年3月20日 20:53
> > To: Ye, MingjinX <mingjinx.ye@intel.com>; dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; stable@dpdk.org; Zhou,
> > YidingX <yidingx.zhou@intel.com>; Zhang, Ke1X <ke1x.zhang@intel.com>
> > Subject: RE: [PATCH v5] net/ice: fix ice dcf control thread crash
> 
> > >  	for (;;) {
> > > +		if (hw->vc_event_msg_cb == NULL)
> > > +			break;
> > Can you explain why this is required, seems it not related with your
> > commit log
> The purpose of this is to bring all 'ice-reset' threads to a quick end when hw
> is released.

I don't understand, the vc_event_msg_cb was initialized in ice_dcf_dev_init and never be reset why we need this check, anything I missed?
> 
> > >
> > > -	rte_intr_enable(pci_dev->intr_handle);
> > > -	ice_dcf_enable_irq0(hw);
> > > +	if (hw->vc_event_msg_cb != NULL) {
> > > +		rte_intr_enable(pci_dev->intr_handle);
> > > +		ice_dcf_enable_irq0(hw);
> >
> > Same question as above
> These are called when HW releases the resource. Therefore, there is no need
> to call.
> 
> > > +	rte_spinlock_lock(&dcf_hw->vsi_thread_lock);
> > > +	dcf_hw->vsi_update_thread_num++;
> > > +	rte_spinlock_unlock(&dcf_hw->vsi_thread_lock);
> >
> > I think you can define vsi_update_thread_num as rte_atomic32_t and use
> > rte_atomic32_add/sub
> At first I chose the rte_atomic32_t option, which is not very elegant using
> spinlock.
> The 'checkpatches.sh' script gives a warning ('Using rte_atomicNN_xxx') when
> it is executed. I saw the comment on line 89 of the script (# refrain from new
> additions of 16/32/64 bits rte_atomicNN_xxx()), so I went with the spinlock
> solution.

You are right, rte_atomicNN_xxx will be deprecated, and should not be used
We can use the gcc build-in function __atomic_fetch_add/sub as an alternative, sp

> 
>