On Fri, Jun 2, 2023 at 6:22 PM Tyler Retzlaff <roret...@linux.microsoft.com> wrote: > On Thu, Jun 01, 2023 at 12:23:39PM +0000, Zhang, Qi Z wrote: > > The patchset looks good to me. > > > > I have just one question regarding the patch set targets, which include PMD > > iavf, ice, and ixgbe. However, I noticed that some other Intel PMDs like > > ipn3ke still use rte_ctrl_thread_create and have not been replaced. > > The series really isn't about rte_ctrl_thread_create, it just happens > that for code built on Windows that code needs to stop using > rte_ctrl_thread_create because it references the pthread shim that is > being removed. > > At some point in the future (it's lower priority right now) I will > submit a series that converts all rte_ctrl_thread_create -> > rte_control_thread_create since rte_ctrl_thread_create is being > deprecated. > > > > > I assume that this replacement is specifically intended for PMDs that > > support Windows, as PMDs with the "Windows" feature should be covered. > > However, I didn't see the "Windows" feature enabled for iavf PMD, even > > though it is included in the patch set. > > > > Could you help me understand the criteria used for determining which PMDs > > should be included in this replacement? > > Yes, you are correct the patch is intended to address PMDs / code built > on Windows specifically. I just re-verified that iavf is being built > for Windows. > > If I remove the iavf patch from the series I get the following warning, > so that is why I adapted the iavf PMD code. > > [249/749] Compiling C object > drivers/libtmp_rte_net_iavf.a.p/net_iavf_iavf_vchnl.c.obj > ../drivers/net/iavf/iavf_vchnl.c:162:2: warning: implicit declaration of > function 'pthread_join' is invalid in C99 > [-Wimplicit-function-declaration] > pthread_join(handler->tid, NULL); > > > > > Thanks > > Qi > > > > > > > > The changes are straightforward and lgtm. > > > For the series, > > > Reviewed-by: David Marchand <david.march...@redhat.com> > > I think with the above explained the series should be okay as is, no > changes required if Qi is okay with the above explanation.
Which seems to be the case. Thank you. -- David Marchand
