On Tue, Mar 21, 2023 at 11:55:19AM +0000, Zhang, Qi Z wrote:
>
>
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx...@intel.com>
> > Sent: Tuesday, March 21, 2023 10:08 AM
> > To: Zhang, Qi Z <qi.z.zh...@intel.com>; dev@dpdk.org
> > Cc: Yang, Qiming <qiming.y...@intel.com>; sta...@dpdk.org; Zhou, YidingX
> > <yidingx.z...@intel.com>; Zhang, Ke1X <ke1x.zh...@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.zh...@intel.com>
> > > Sent: 2023年3月20日 20:53
> > > To: Ye, MingjinX <mingjinx...@intel.com>; dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.y...@intel.com>; sta...@dpdk.org; Zhou,
> > > YidingX <yidingx.z...@intel.com>; Zhang, Ke1X <ke1x.zh...@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.
>
> >
> >
