Re: [PATCH 0/6] windows: remove most pthread lifetime shim functions

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

On Thu, Jun 01, 2023 at 12:23:39PM +0000, Zhang, Qi Z wrote:
> 
> 
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Thursday, June 1, 2023 4:50 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>; Tyler Retzlaff
> > <roretzla@linux.microsoft.com>
> > Cc: dev@dpdk.org; thomas@monjalon.net; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Subject: Re: [PATCH 0/6] windows: remove most pthread lifetime shim
> > functions
> > 
> > On Tue, Apr 18, 2023 at 11:21 AM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Sun, Apr 02, 2023 at 10:34:12PM -0700, Tyler Retzlaff wrote:
> > > > early review if possible please, would like to have this in from the
> > > > start of 23.07 to work against.
> > > >
> > > > thanks!
> > > >
> > >
> > > Don't see any problems with this set.
> > 
> > Drivers maintainers were not copied (Tyler, git send-email has options --to-
> > cmd or --cc-cmd to which you can pass ./devtools/get-maintainers.sh).
> > I pinged Qi during the maintainers call today.
> 
> Hi Tyler & David:
> 
> 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.marchand@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.

> > 
> > 
> > --
> > David Marchand
>