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

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Tue, Mar 21, 2023 at 11:55:19AM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----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

using __atomic_fetch_{add,sub} is appropriate. please be aware using
__atomic_{add_fetch}_fetch is discouraged but it is easy to write the
code using only the former.

> 
> > 
> >