diff mbox series

[v2,4/6] net/pcap: add libpcap wrappers

Message ID 20210214021616.26970-5-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series net/pcap: build on Windows | expand

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Dmitry Kozlyuk Feb. 14, 2021, 2:16 a.m. UTC
libpcap headers can expose OS headers. On Windows, system networking
headers are incompatible with DPDK ones, causing multiple name clashes.
API of libpcap itself involves a non-standard u_char type.

Add a limited set of trivial libpcap wrappers, so that libpcap headers
are not included directly by OS-independent PMD code. Use EAL types and
functions for time instead of POSIX ones.

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 drivers/net/pcap/meson.build   |   1 +
 drivers/net/pcap/pcap_ethdev.c | 157 +++++++++++++++++----------------
 drivers/net/pcap/pcap_osdep.c  | 122 +++++++++++++++++++++++++
 drivers/net/pcap/pcap_osdep.h  |  50 +++++++++++
 4 files changed, 252 insertions(+), 78 deletions(-)
 create mode 100644 drivers/net/pcap/pcap_osdep.c

Comments

Ferruh Yigit Feb. 25, 2021, 2:59 p.m. UTC | #1
On 2/14/2021 2:16 AM, Dmitry Kozlyuk wrote:
> libpcap headers can expose OS headers. On Windows, system networking
> headers are incompatible with DPDK ones, causing multiple name clashes.
> API of libpcap itself involves a non-standard u_char type.
> 
> Add a limited set of trivial libpcap wrappers, so that libpcap headers
> are not included directly by OS-independent PMD code. Use EAL types and
> functions for time instead of POSIX ones.
> 

It is not nice to duplicate the pcap struct and macros and have dpdk versions of 
them, it may have long term maintanance issues too.

What are the clashes in question?

If they are macros, can the issue solved after undefining some after including 
'pcap.h'?

And at the end of the day, shouldn't we need to resolve these clashes globally 
since we may hit same things with other PMDs or libraries as we enable them. I 
am for a global solution instead of these changes in pcap, but I guess question 
is how possible that global solution is..

> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

<...>
Dmitry Kozlyuk Feb. 25, 2021, 7:04 p.m. UTC | #2
2021-02-25 14:59, Ferruh Yigit:
> On 2/14/2021 2:16 AM, Dmitry Kozlyuk wrote:
> > libpcap headers can expose OS headers. On Windows, system networking
> > headers are incompatible with DPDK ones, causing multiple name clashes.
> > API of libpcap itself involves a non-standard u_char type.
> > 
> > Add a limited set of trivial libpcap wrappers, so that libpcap headers
> > are not included directly by OS-independent PMD code. Use EAL types and
> > functions for time instead of POSIX ones.
> >   
> 
> It is not nice to duplicate the pcap struct and macros and have dpdk versions of 
> them, it may have long term maintanance issues too.
> 
> What are the clashes in question?
> 
> If they are macros, can the issue solved after undefining some after including 
> 'pcap.h'?
> 
> And at the end of the day, shouldn't we need to resolve these clashes globally 
> since we may hit same things with other PMDs or libraries as we enable them. I 
> am for a global solution instead of these changes in pcap, but I guess question 
> is how possible that global solution is..

Your comment made me revise Windows EAL networking shims. Surprisingly, if
changed to expose Windows networking headers (specifically, <ws2tcpip.h>) with
some additions (but no hacks), they create no issues to any existing code.

The only workaround remaining is `#undef s_addr` in <rte_ether.h>.

So maybe this commit can be dropped, if Windows EAL networking headers be
reworked in the following way:

	#if defined min
	#define USER_EXPLICITLY_WANTS_WINDOWS_H
	#endif

	#include <ws2tcpip.h>

	/* hide definitions that break portable code,
	 * e.g. it had to be done once already for i40e
	 */
	#ifndef USER_EXPLICITLY_WANTS_WINDOWS_H
	#undef min, max, ...
	#endif

	#define what's missing from Windows headers, e.g. IPPROTO_SCTP

+ Windows maintainers, Nick Connolly, and Jie Zhou to discuss.
Nick Connolly Feb. 25, 2021, 8:31 p.m. UTC | #3
> Your comment made me revise Windows EAL networking shims. Surprisingly, if
> changed to expose Windows networking headers (specifically, <ws2tcpip.h>) with
> some additions (but no hacks), they create no issues to any existing code.
>
> The only workaround remaining is `#undef s_addr` in <rte_ether.h>.
>
> So maybe this commit can be dropped, if Windows EAL networking headers be
> reworked in the following way:
>
> 	#if defined min
> 	#define USER_EXPLICITLY_WANTS_WINDOWS_H
> 	#endif
>
> 	#include <ws2tcpip.h>
>
> 	/* hide definitions that break portable code,
> 	 * e.g. it had to be done once already for i40e
> 	 */
> 	#ifndef USER_EXPLICITLY_WANTS_WINDOWS_H
> 	#undef min, max, ...
> 	#endif
>
> 	#define what's missing from Windows headers, e.g. IPPROTO_SCTP
>
> + Windows maintainers, Nick Connolly, and Jie Zhou to discuss.
In my opinion, there are long term maintenance issues either way round.

Wrapping the system headers hides the details and keeps the code 'clean',
but has to work with multiple compiler environments and versions of the
headers - not always straightforward.  Wrapping libpcap involves code
changes that need to be maintained.

For SPDK, changing the code wasn't really an option, so I had to go with
wrapping the headers. It's worked out ok so far. To make it a bit cleaner
I turned off as much as possible from Windows.h with #define NO*.
Supporting both clang and MinGW caused a few issues (e.g. MinGW has
mswsock.h), but they are solvable.

./inc/sys/socket.h and ./inc/wpdk/windows.h in https://github.com/wpdp/wpdk
show an example of what might need wrapping in the general case.

Regards,
Nick
Dmitry Kozlyuk Feb. 25, 2021, 11:10 p.m. UTC | #4
2021-02-25 20:31, Nick Connolly:
> > Your comment made me revise Windows EAL networking shims. Surprisingly, if
> > changed to expose Windows networking headers (specifically, <ws2tcpip.h>) with
> > some additions (but no hacks), they create no issues to any existing code.
> >
> > The only workaround remaining is `#undef s_addr` in <rte_ether.h>.
> >
> > So maybe this commit can be dropped, if Windows EAL networking headers be
> > reworked in the following way:
> >
> > 	#if defined min
> > 	#define USER_EXPLICITLY_WANTS_WINDOWS_H
> > 	#endif
> >
> > 	#include <ws2tcpip.h>
> >
> > 	/* hide definitions that break portable code,
> > 	 * e.g. it had to be done once already for i40e
> > 	 */
> > 	#ifndef USER_EXPLICITLY_WANTS_WINDOWS_H
> > 	#undef min, max, ...
> > 	#endif
> >
> > 	#define what's missing from Windows headers, e.g. IPPROTO_SCTP
> >
> > + Windows maintainers, Nick Connolly, and Jie Zhou to discuss.  
> In my opinion, there are long term maintenance issues either way round.
> 
> Wrapping the system headers hides the details and keeps the code 'clean',
> but has to work with multiple compiler environments and versions of the
> headers - not always straightforward. 

You're probably right.
We had an exchange with Tyler, let me quote:

[DmitryK]
> I'm pretty sure no DPDK header really needs anything beyond standard C,
> but one exception: intrinsic functions for vector instructions.
> Struct in_addr? An oversight, there's rte_ether_addr, should be rte_ip_addr.

Here's a complete list of offending files included from public headers,
with comments on usage:

POSIX

    netinet/in.h
    netinet/ip6.h
    netinet/ip.h
	cmdline:  struct in_addr, struct in6_addr, 6 constants
	net:      ditto
	security: ditto
    pthread.h
	eal:	not used
	ethdev:	pthread_mutex_t flow_ops_mutex;
    sched.h
	eal:       cpu_set_t
	telemetry: rte_cpuset_t (not used?)
    sys/types.h
	ehtdev:   not needed (FILE?)
	net:      not needed
	mbuf:     ditto
	pci:      ditto
	security: ditto
	sched:    ditto

Unix

    sys/queue.h
	(acl, eal, hash, lpm, mempool, pci, ring, table,
	 bus/{fslmc,pci,vdev,vmbus}, net/{memif,tap})
		multiprocessing

Thread-related stuff will go away as rte_thread.h expands.
Network headers are not much needed, as you can see above.
Other headers mostly not needed or can be replaced with standard ones.

Replacing `in_addr` with and `rte_ip_addr` will break API, but not ABI,
because it's essentially the same thing. We can define new types
conditionally, as it was done for struct cmdline, if need be.

Complete removal of non-standard dependencies in headers is within a grasp.
Then we can remove shims and include whatever needed.
Thoughts?
Nick Connolly March 1, 2021, 9:43 p.m. UTC | #5
> Complete removal of non-standard dependencies in headers is within a grasp.
> Then we can remove shims and include whatever needed.
> Thoughts?
Sounds good. A couple of 'gotchas' that I've come across (but may not be 
an issue for DPDK):

  * Memory allocation / free that spans a dll boundary (see earlier email).
  * posix_memalign is unfortunately specified as using free(), so we'll
    either have to modify the calls to rte_posix_memalign /
    rte_free_memalign (which means code analysis to find which free
    calls need changing), or wrapper free() to distinguish the two types
    of memory somehow. I 'solved' this for SPDK by using posix_memalign
    for everything and I have a vague recollection that the mingw libc
    does something similar.
  * Sockets are unfortunately specified as using close(). This is
    probably easy to address by rte_ wrapping all socket calls.

Regards,
Nick
Dmitry Kozlyuk March 1, 2021, 11:05 p.m. UTC | #6
2021-03-01 21:43, Nick Connolly:
> > Complete removal of non-standard dependencies in headers is within a grasp.
> > Then we can remove shims and include whatever needed.
> > Thoughts?
> Sounds good. A couple of 'gotchas' that I've come across (but may not be 
> an issue for DPDK):
> 
>   * Memory allocation / free that spans a dll boundary (see earlier email).

Not sure which email you mean, could you give a link?

>   * posix_memalign is unfortunately specified as using free(), so we'll
>     either have to modify the calls to rte_posix_memalign /
>     rte_free_memalign (which means code analysis to find which free
>     calls need changing), or wrapper free() to distinguish the two types
>     of memory somehow. I 'solved' this for SPDK by using posix_memalign
>     for everything and I have a vague recollection that the mingw libc
>     does something similar.

Is posix_memalign() used more extensively in SPDK? In DPDK, it's 2 PMDs:
  * mlx5 PMD uses _aligned_malloc/_aligned_free on Windows;
  * dpaax (PMD family) uses posix_memalign() to allocate pages.
There are "malloc" and "alloc_size" attributes that can help code analysis.

>   * Sockets are unfortunately specified as using close(). This is
>     probably easy to address by rte_ wrapping all socket calls.

Which public DPDK APIs operate on sockets?
I don't like the idea of wrapping APIs like sockets or files.
(Yes, we're discussing libpcap API wrappers in this thread, but we already
agreed they are a mistake and they were internal in the first place.)

I drafted what I was talking about: adding address types and removing shims:

* librte_net/rte_ip.h then includes <netinet/ip.h> or <ws2tcpip.h>
  conditionally for AF_xxx, IPPROTO_xxx, and a few other constants.
  That's probably OK, there are similar places for Linux/FreeBSD differences,
  e.g. in <rte_endian.h>.

* Some IPPROTO_xxx constants are missing on Windows, so rte_ip.h has to
  provide them. I hope Mirosoft will add them to system headers one day.

* It affects cmdline (mostly), ethdev, security, crypto/dpaax.
Dmitry Kozlyuk March 1, 2021, 11:23 p.m. UTC | #7
2021-03-02 02:05, Dmitry Kozlyuk:
> 2021-03-01 21:43, Nick Connolly:
> > > Complete removal of non-standard dependencies in headers is within a grasp.
> > > Then we can remove shims and include whatever needed.
> > > Thoughts?  
> > Sounds good. A couple of 'gotchas' that I've come across (but may not be 
> > an issue for DPDK):
> > 
> >   * Memory allocation / free that spans a dll boundary (see earlier email).  
> 
> Not sure which email you mean, could you give a link?

There it is:

	https://mails.dpdk.org/archives/dev/2021-February/200105.html

Yes, of course malloc/free shall be both on either library or caller site.
Nick Connolly March 2, 2021, 11:22 a.m. UTC | #8
> Is posix_memalign() used more extensively in SPDK? In DPDK, it's 2 PMDs:
Yes, there are about 80 references. A lot are in ISA-L where they are 
#defined to _aligned_malloc and can be ignored, but there still several 
in the rest of the code.


>>    * Sockets are unfortunately specified as using close(). This is
>>      probably easy to address by rte_ wrapping all socket calls.
> Which public DPDK APIs operate on sockets?
> I don't like the idea of wrapping APIs like sockets or files.
I'm not sure about use in public APIs - I'd hope none do. I was thinking 
about 'internal' use. The tricky issue here is that on Linux/FreeBSD 
socket() returns a small integer file descriptor whereas Windows returns 
a SOCKET type (which is a UINT_PTR and so larger than the 'int' that an 
fd occupies). In practice it looks as though SOCKET usually returns a 
small integer, but as far as I can see from the documentation it depends 
on the software stack that is in use. Even if it's a small integer, I 
don't think it has any correlation to the file descriptor table in the 
crt, which means that calls to read/write/close will all need to be 
handled so that they work.

> (Yes, we're discussing libpcap API wrappers in this thread, but we already
> agreed they are a mistake and they were internal in the first place.)
>
> I drafted what I was talking about: adding address types and removing shims:
>
> * librte_net/rte_ip.h then includes <netinet/ip.h> or <ws2tcpip.h>
>    conditionally for AF_xxx, IPPROTO_xxx, and a few other constants.
>    That's probably OK, there are similar places for Linux/FreeBSD differences,
>    e.g. in <rte_endian.h>.
>
> * Some IPPROTO_xxx constants are missing on Windows, so rte_ip.h has to
>    provide them. I hope Mirosoft will add them to system headers one day.
>
> * It affects cmdline (mostly), ethdev, security, crypto/dpaax.
Sounds good - well done!

Regards,
Nick
Dmitry Kozlyuk March 3, 2021, 4:32 p.m. UTC | #9
2021-03-02 11:22, Nick Connolly:
> > Is posix_memalign() used more extensively in SPDK? In DPDK, it's 2 PMDs:  
> Yes, there are about 80 references. A lot are in ISA-L where they are 
> #defined to _aligned_malloc and can be ignored, but there still several 
> in the rest of the code.

I think portable code should try sticking to C11 aligned_malloc().
BTW, _aligned_malloc (with "_") only supports power-of-2 alignment.

There's a related passage in MSVC blog:

	Due to the nature of the Windows heap, aligned_alloc support is
	missing. The alternative is to use _aligned_malloc.

https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/

> >>    * Sockets are unfortunately specified as using close(). This is
> >>      probably easy to address by rte_ wrapping all socket calls.  
> > Which public DPDK APIs operate on sockets?
> > I don't like the idea of wrapping APIs like sockets or files.  
> I'm not sure about use in public APIs - I'd hope none do. I was thinking 
> about 'internal' use.

This is important (even more so for SPDK, I suspect), however, I'd like to
focus on public API first.

> > I drafted what I was talking about: adding address types and removing shims:
> >
> > * librte_net/rte_ip.h then includes <netinet/ip.h> or <ws2tcpip.h>
> >    conditionally for AF_xxx, IPPROTO_xxx, and a few other constants.
> >    That's probably OK, there are similar places for Linux/FreeBSD differences,
> >    e.g. in <rte_endian.h>.
> >
> > * Some IPPROTO_xxx constants are missing on Windows, so rte_ip.h has to
> >    provide them. I hope Mirosoft will add them to system headers one day.
> >
> > * It affects cmdline (mostly), ethdev, security, crypto/dpaax.  
> Sounds good - well done!

...or not :)

If we can't help including <ws2tcpip.h>/<netinet/ip.h> from public headers,
might as well use `struct in_addr`, just replace `#include <netinet/ip.h>`
with `#include <rte_ip.h>` everywhere.

The only remaining issue will be `s_addr` macro on Windows that conflicts with
`struct rte_ether_addr` field `s_addr`. I don't know a better solution than
renaming `s_addr` to `src_addr` in DPDK (OTOH, it will be consistent with
`struct rte_ipvX_hdr` field naming).

Ferruh, what do you think?
Ferruh Yigit March 3, 2021, 4:47 p.m. UTC | #10
On 3/3/2021 4:32 PM, Dmitry Kozlyuk wrote:
> 2021-03-02 11:22, Nick Connolly:
>>> Is posix_memalign() used more extensively in SPDK? In DPDK, it's 2 PMDs:
>> Yes, there are about 80 references. A lot are in ISA-L where they are
>> #defined to _aligned_malloc and can be ignored, but there still several
>> in the rest of the code.
> 
> I think portable code should try sticking to C11 aligned_malloc().
> BTW, _aligned_malloc (with "_") only supports power-of-2 alignment.
> 
> There's a related passage in MSVC blog:
> 
> 	Due to the nature of the Windows heap, aligned_alloc support is
> 	missing. The alternative is to use _aligned_malloc.
> 
> https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/
> 
>>>>     * Sockets are unfortunately specified as using close(). This is
>>>>       probably easy to address by rte_ wrapping all socket calls.
>>> Which public DPDK APIs operate on sockets?
>>> I don't like the idea of wrapping APIs like sockets or files.
>> I'm not sure about use in public APIs - I'd hope none do. I was thinking
>> about 'internal' use.
> 
> This is important (even more so for SPDK, I suspect), however, I'd like to
> focus on public API first.
> 
>>> I drafted what I was talking about: adding address types and removing shims:
>>>
>>> * librte_net/rte_ip.h then includes <netinet/ip.h> or <ws2tcpip.h>
>>>     conditionally for AF_xxx, IPPROTO_xxx, and a few other constants.
>>>     That's probably OK, there are similar places for Linux/FreeBSD differences,
>>>     e.g. in <rte_endian.h>.
>>>
>>> * Some IPPROTO_xxx constants are missing on Windows, so rte_ip.h has to
>>>     provide them. I hope Mirosoft will add them to system headers one day.
>>>
>>> * It affects cmdline (mostly), ethdev, security, crypto/dpaax.
>> Sounds good - well done!
> 
> ...or not :)
> 
> If we can't help including <ws2tcpip.h>/<netinet/ip.h> from public headers,
> might as well use `struct in_addr`, just replace `#include <netinet/ip.h>`
> with `#include <rte_ip.h>` everywhere.
> 
> The only remaining issue will be `s_addr` macro on Windows that conflicts with
> `struct rte_ether_addr` field `s_addr`. I don't know a better solution than
> renaming `s_addr` to `src_addr` in DPDK (OTOH, it will be consistent with
> `struct rte_ipvX_hdr` field naming).
> 
> Ferruh, what do you think?
> 

No problem on the chosen name, that will work fine, but the 'struct 
rte_eth_addr' is public struct, we can't rename it without breaking the user 
applications.

Still it is possible to rename public struct, but it is a long process, need to 
send a deprecation notice and wait for next LTS, 21.11.

Ethernet header already has following, doesn't it help:
#undef s_addr /* Defined in winsock2.h included in windows.h. */
Dmitry Kozlyuk March 3, 2021, 6:19 p.m. UTC | #11
2021-03-03 16:47, Ferruh Yigit:
> On 3/3/2021 4:32 PM, Dmitry Kozlyuk wrote:
[...]
> > If we can't help including <ws2tcpip.h>/<netinet/ip.h> from public headers,
> > might as well use `struct in_addr`, just replace `#include <netinet/ip.h>`
> > with `#include <rte_ip.h>` everywhere.
> > 
> > The only remaining issue will be `s_addr` macro on Windows that conflicts with
> > `struct rte_ether_addr` field `s_addr`. I don't know a better solution than
> > renaming `s_addr` to `src_addr` in DPDK (OTOH, it will be consistent with
> > `struct rte_ipvX_hdr` field naming).
> > 
> > Ferruh, what do you think?
> >   
> 
> No problem on the chosen name, that will work fine, but the 'struct 
> rte_eth_addr' is public struct, we can't rename it without breaking the user 
> applications.
> 
> Still it is possible to rename public struct, but it is a long process, need to 
> send a deprecation notice and wait for next LTS, 21.11.
> 
> Ethernet header already has following, doesn't it help:
> #undef s_addr /* Defined in winsock2.h included in windows.h. */

It helps until you do the following:

	#include <winsock2.h>	/* #define s_addr S_un.S_addr */
	#include <rte_ether.h>	/* #undef s_addr */

	struct in_addr a;
	a.s_addr = 0; /* ERROR, `s_addr` has been defined to do that */

Or the following:

	#include <rte_ether.h>
	#include <winsock2.h>

	struct rte_ether_hdr eh;
	eh.s_addr.addr_bytes[0] = 0;	/* ERROR: `addr_s` is a macro */

If you swap include order in each case, it compiles, i.e . code is fragile.
Shims redefine `struct in_addr` and we're trying to get rid of them anyway.


Here's a solution that always works, however ugly it looks. It defines
`struct rte_ether_hdr` for Windows such that `s_addr` macro works for it if
defined. We can keep it until 21.11, then remove it and rename fields. In
return, we can remove networking shims and workarounds immediately.

`S_un.S_addr` part of `struct in_addr` is documented, and thus is unlikely to
ever change:

	https://docs.microsoft.com/en-us/windows/win32/api/winsock2/ns-winsock2-in_addr


diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 060b63fc9..67d340bb2 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -23,10 +23,6 @@ extern "C" {
 #include <rte_mbuf.h>
 #include <rte_byteorder.h>
 
-#ifdef RTE_EXEC_ENV_WINDOWS /* Workaround conflict with rte_ether_hdr. */
-#undef s_addr /* Defined in winsock2.h included in windows.h. */
-#endif
-
 #define RTE_ETHER_ADDR_LEN  6 /**< Length of Ethernet address. */
 #define RTE_ETHER_TYPE_LEN  2 /**< Length of Ethernet type field. */
 #define RTE_ETHER_CRC_LEN   4 /**< Length of Ethernet CRC. */
@@ -257,6 +253,8 @@ __rte_experimental
 int
 rte_ether_unformat_addr(const char *str, struct rte_ether_addr *eth_addr);
 
+#if !defined RTE_EXEC_ENV_WINDOWS || defined __DOXYGEN__
+
 /**
  * Ethernet header: Contains the destination address, source address
  * and frame type.
@@ -267,6 +265,28 @@ struct rte_ether_hdr {
 	uint16_t ether_type;      /**< Frame type. */
 } __rte_aligned(2);
 
+#else
+
+#pragma push_macro("s_addr")
+#ifdef s_addr
+#undef s_addr
+#endif
+
+struct rte_ether_hdr {
+	struct rte_ether_addr d_addr; /**< Destination address. */
+	RTE_STD_C11
+	union {
+		struct rte_ether_addr s_addr; /**< Source address. */
+		struct {
+			struct rte_ether_addr S_un;
+		} S_addr;
+	};
+	uint16_t ether_type; /**< Frame type. */
+} __rte_aligned(2);
+
+#pragma pop_macro("s_addr")
+#endif
+
 /**
  * Ethernet VLAN Header.
  * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type
Ferruh Yigit March 3, 2021, 7:30 p.m. UTC | #12
On 3/3/2021 6:19 PM, Dmitry Kozlyuk wrote:
> 2021-03-03 16:47, Ferruh Yigit:
>> On 3/3/2021 4:32 PM, Dmitry Kozlyuk wrote:
> [...]
>>> If we can't help including <ws2tcpip.h>/<netinet/ip.h> from public headers,
>>> might as well use `struct in_addr`, just replace `#include <netinet/ip.h>`
>>> with `#include <rte_ip.h>` everywhere.
>>>
>>> The only remaining issue will be `s_addr` macro on Windows that conflicts with
>>> `struct rte_ether_addr` field `s_addr`. I don't know a better solution than
>>> renaming `s_addr` to `src_addr` in DPDK (OTOH, it will be consistent with
>>> `struct rte_ipvX_hdr` field naming).
>>>
>>> Ferruh, what do you think?
>>>    
>>
>> No problem on the chosen name, that will work fine, but the 'struct
>> rte_eth_addr' is public struct, we can't rename it without breaking the user
>> applications.
>>
>> Still it is possible to rename public struct, but it is a long process, need to
>> send a deprecation notice and wait for next LTS, 21.11.
>>
>> Ethernet header already has following, doesn't it help:
>> #undef s_addr /* Defined in winsock2.h included in windows.h. */
> 
> It helps until you do the following:
> 
> 	#include <winsock2.h>	/* #define s_addr S_un.S_addr */
> 	#include <rte_ether.h>	/* #undef s_addr */
> 
> 	struct in_addr a;
> 	a.s_addr = 0; /* ERROR, `s_addr` has been defined to do that */
> 
> Or the following:
> 
> 	#include <rte_ether.h>
> 	#include <winsock2.h>
> 
> 	struct rte_ether_hdr eh;
> 	eh.s_addr.addr_bytes[0] = 0;	/* ERROR: `addr_s` is a macro */
> 
> If you swap include order in each case, it compiles, i.e . code is fragile.
> Shims redefine `struct in_addr` and we're trying to get rid of them anyway.
> 

Got it, thanks for clarification.

> 
> Here's a solution that always works, however ugly it looks. It defines
> `struct rte_ether_hdr` for Windows such that `s_addr` macro works for it if
> defined. We can keep it until 21.11, then remove it and rename fields. In
> return, we can remove networking shims and workarounds immediately.
> 

Agree it looks ugly :), but I am OK with above plan.
You need to send a deprecation notice for this. We can discuss there if 'd_addr' 
also should be renamed for consistency, or leave with the workaround instead of 
rename ...

> `S_un.S_addr` part of `struct in_addr` is documented, and thus is unlikely to
> ever change:
> 
> 	https://docs.microsoft.com/en-us/windows/win32/api/winsock2/ns-winsock2-in_addr
> 
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 060b63fc9..67d340bb2 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -23,10 +23,6 @@ extern "C" {
>   #include <rte_mbuf.h>
>   #include <rte_byteorder.h>
>   
> -#ifdef RTE_EXEC_ENV_WINDOWS /* Workaround conflict with rte_ether_hdr. */
> -#undef s_addr /* Defined in winsock2.h included in windows.h. */
> -#endif
> -
>   #define RTE_ETHER_ADDR_LEN  6 /**< Length of Ethernet address. */
>   #define RTE_ETHER_TYPE_LEN  2 /**< Length of Ethernet type field. */
>   #define RTE_ETHER_CRC_LEN   4 /**< Length of Ethernet CRC. */
> @@ -257,6 +253,8 @@ __rte_experimental
>   int
>   rte_ether_unformat_addr(const char *str, struct rte_ether_addr *eth_addr);
>   
> +#if !defined RTE_EXEC_ENV_WINDOWS || defined __DOXYGEN__
> +
>   /**
>    * Ethernet header: Contains the destination address, source address
>    * and frame type.
> @@ -267,6 +265,28 @@ struct rte_ether_hdr {
>   	uint16_t ether_type;      /**< Frame type. */
>   } __rte_aligned(2);
>   
> +#else
> +
> +#pragma push_macro("s_addr")
> +#ifdef s_addr
> +#undef s_addr
> +#endif

I guess this needs to be as following, in case this header included before the 
windows header:

#ifdef s_addr
#pragma push_macro("s_addr")
#undef s_addr
#endif

> +
> +struct rte_ether_hdr {
> +	struct rte_ether_addr d_addr; /**< Destination address. */
> +	RTE_STD_C11
> +	union {
> +		struct rte_ether_addr s_addr; /**< Source address. */
> +		struct {
> +			struct rte_ether_addr S_un;
> +		} S_addr;
> +	};
> +	uint16_t ether_type; /**< Frame type. */
> +} __rte_aligned(2);
> +
> +#pragma pop_macro("s_addr")
> +#endif
> +
>   /**
>    * Ethernet VLAN Header.
>    * Contains the 16-bit VLAN Tag Control Identifier and the Ethernet type
>
Dmitry Kozlyuk March 3, 2021, 11:03 p.m. UTC | #13
2021-03-03 19:30, Ferruh Yigit:
> On 3/3/2021 6:19 PM, Dmitry Kozlyuk wrote:
> > 2021-03-03 16:47, Ferruh Yigit:  
> >> On 3/3/2021 4:32 PM, Dmitry Kozlyuk wrote:  
[...]
> > +#pragma push_macro("s_addr")
> > +#ifdef s_addr
> > +#undef s_addr
> > +#endif  
> 
> I guess this needs to be as following, in case this header included before the 
> windows header:
> 
> #ifdef s_addr
> #pragma push_macro("s_addr")
> #undef s_addr
> #endif

It actually works both ways.
I used #ifdef inside so that #pragma pop_macro doesn't need its own guard.
diff mbox series

Patch

diff --git a/drivers/net/pcap/meson.build b/drivers/net/pcap/meson.build
index 26b606699..9ab95ec3e 100644
--- a/drivers/net/pcap/meson.build
+++ b/drivers/net/pcap/meson.build
@@ -13,6 +13,7 @@  if not dpdk_conf.has('RTE_PORT_PCAP')
 endif
 sources = files(
 	'pcap_ethdev.c',
+	'pcap_osdep.c',
 	'pcap_osdep_@0@.c'.format(exec_env),
 )
 ext_deps += pcap_dep
diff --git a/drivers/net/pcap/pcap_ethdev.c b/drivers/net/pcap/pcap_ethdev.c
index a102897e9..6461213e0 100644
--- a/drivers/net/pcap/pcap_ethdev.c
+++ b/drivers/net/pcap/pcap_ethdev.c
@@ -4,17 +4,13 @@ 
  * All rights reserved.
  */
 
-#include <time.h>
-
-#include <pcap.h>
-
-#include <rte_cycles.h>
 #include <ethdev_driver.h>
 #include <ethdev_vdev.h>
+#include <rte_bus_vdev.h>
+#include <rte_cycles.h>
 #include <rte_kvargs.h>
 #include <rte_malloc.h>
 #include <rte_mbuf.h>
-#include <rte_bus_vdev.h>
 
 #include "pcap_osdep.h"
 
@@ -36,8 +32,8 @@ 
 
 #define RTE_PMD_PCAP_MAX_QUEUES 16
 
-static char errbuf[PCAP_ERRBUF_SIZE];
-static struct timeval start_time;
+static char errbuf[OSDEP_PCAP_ERRBUF_SIZE];
+static struct rte_time_us start_time;
 static uint64_t start_cycles;
 static uint64_t hz;
 static uint8_t iface_idx;
@@ -83,16 +79,16 @@  struct pmd_internals {
 };
 
 struct pmd_process_private {
-	pcap_t *rx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
-	pcap_t *tx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
-	pcap_dumper_t *tx_dumper[RTE_PMD_PCAP_MAX_QUEUES];
+	osdep_pcap * rx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
+	osdep_pcap * tx_pcap[RTE_PMD_PCAP_MAX_QUEUES];
+	osdep_pcap_dumper * tx_dumper[RTE_PMD_PCAP_MAX_QUEUES];
 };
 
 struct pmd_devargs {
 	unsigned int num_of_queue;
 	struct devargs_queue {
-		pcap_dumper_t *dumper;
-		pcap_t *pcap;
+		osdep_pcap_dumper *dumper;
+		osdep_pcap *pcap;
 		const char *name;
 		const char *type;
 	} queue[RTE_PMD_PCAP_MAX_QUEUES];
@@ -137,7 +133,7 @@  RTE_LOG_REGISTER(eth_pcap_logtype, pmd.net.pcap, NOTICE);
 
 static int
 eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
-		const u_char *data, uint16_t data_len)
+		const uint8_t *data, uint16_t data_len)
 {
 	/* Copy the first segment. */
 	uint16_t len = rte_pktmbuf_tailroom(mbuf);
@@ -214,14 +210,14 @@  static uint16_t
 eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 {
 	unsigned int i;
-	struct pcap_pkthdr header;
+	struct osdep_pcap_pkthdr header;
 	struct pmd_process_private *pp;
-	const u_char *packet;
+	const uint8_t *packet;
 	struct rte_mbuf *mbuf;
 	struct pcap_rx_queue *pcap_q = queue;
 	uint16_t num_rx = 0;
 	uint32_t rx_bytes = 0;
-	pcap_t *pcap;
+	osdep_pcap *pcap;
 
 	pp = rte_eth_devices[pcap_q->port_id].process_private;
 	pcap = pp->rx_pcap[pcap_q->queue_id];
@@ -234,7 +230,7 @@  eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	 */
 	for (i = 0; i < nb_pkts; i++) {
 		/* Get the next PCAP packet */
-		packet = pcap_next(pcap, &header);
+		packet = osdep_pcap_next(pcap, &header);
 		if (unlikely(packet == NULL))
 			break;
 
@@ -261,8 +257,8 @@  eth_pcap_rx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		mbuf->pkt_len = (uint16_t)header.caplen;
 		*RTE_MBUF_DYNFIELD(mbuf, timestamp_dynfield_offset,
 			rte_mbuf_timestamp_t *) =
-				(uint64_t)header.ts.tv_sec * 1000000 +
-				header.ts.tv_usec;
+				(uint64_t)header.ts.sec * 1000000 +
+				header.ts.usec;
 		mbuf->ol_flags |= timestamp_rx_dynflag;
 		mbuf->port = pcap_q->port_id;
 		bufs[num_rx] = mbuf;
@@ -285,20 +281,24 @@  eth_null_rx(void *queue __rte_unused,
 
 #define NSEC_PER_SEC	1000000000L
 
+/*
+ * This function stores nanoseconds in "usec" field of struct rte_time_us,
+ * because "ts" goes directly to nanosecond-precision dump.
+ */
 static inline void
-calculate_timestamp(struct timeval *ts) {
+calculate_timestamp(struct rte_time_us *ts) {
 	uint64_t cycles;
-	struct timeval cur_time;
+	struct rte_time_us cur_time;
 
 	cycles = rte_get_timer_cycles() - start_cycles;
-	cur_time.tv_sec = cycles / hz;
-	cur_time.tv_usec = (cycles % hz) * NSEC_PER_SEC / hz;
-
-	ts->tv_sec = start_time.tv_sec + cur_time.tv_sec;
-	ts->tv_usec = start_time.tv_usec + cur_time.tv_usec;
-	if (ts->tv_usec >= NSEC_PER_SEC) {
-		ts->tv_usec -= NSEC_PER_SEC;
-		ts->tv_sec += 1;
+	cur_time.sec = cycles / hz;
+	cur_time.usec = (cycles % hz) * NSEC_PER_SEC / hz;
+
+	ts->sec = start_time.sec + cur_time.sec;
+	ts->usec = start_time.usec + cur_time.usec;
+	if (ts->usec >= NSEC_PER_SEC) {
+		ts->usec -= NSEC_PER_SEC;
+		ts->sec += 1;
 	}
 }
 
@@ -314,8 +314,8 @@  eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct pcap_tx_queue *dumper_q = queue;
 	uint16_t num_tx = 0;
 	uint32_t tx_bytes = 0;
-	struct pcap_pkthdr header;
-	pcap_dumper_t *dumper;
+	struct osdep_pcap_pkthdr header;
+	osdep_pcap_dumper *dumper;
 	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
 	size_t len, caplen;
 
@@ -342,7 +342,7 @@  eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
 		 * a pointer to temp_data after copying into it.
 		 */
-		pcap_dump((u_char *)dumper, &header,
+		osdep_pcap_dump((uint8_t *)dumper, &header,
 			rte_pktmbuf_read(mbuf, 0, caplen, temp_data));
 
 		num_tx++;
@@ -355,7 +355,7 @@  eth_pcap_tx_dumper(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	 * process stops and to make sure the pcap file is actually written,
 	 * we flush the pcap dumper within each burst.
 	 */
-	pcap_dump_flush(dumper);
+	osdep_pcap_dump_flush(dumper);
 	dumper_q->tx_stat.pkts += num_tx;
 	dumper_q->tx_stat.bytes += tx_bytes;
 	dumper_q->tx_stat.err_pkts += nb_pkts - num_tx;
@@ -400,7 +400,7 @@  eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	struct pcap_tx_queue *tx_queue = queue;
 	uint16_t num_tx = 0;
 	uint32_t tx_bytes = 0;
-	pcap_t *pcap;
+	osdep_pcap *pcap;
 	unsigned char temp_data[RTE_ETH_PCAP_SNAPLEN];
 	size_t len;
 
@@ -426,7 +426,7 @@  eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 		 * in the mbuf (when the mbuf is contiguous) or, otherwise,
 		 * a pointer to temp_data after copying into it.
 		 */
-		ret = pcap_sendpacket(pcap,
+		ret = osdep_pcap_sendpacket(pcap,
 			rte_pktmbuf_read(mbuf, 0, len, temp_data), len);
 		if (unlikely(ret != 0))
 			break;
@@ -443,11 +443,11 @@  eth_pcap_tx(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 }
 
 /*
- * pcap_open_live wrapper function
+ * osdep_pcap_open_live wrapper function
  */
 static inline int
-open_iface_live(const char *iface, pcap_t **pcap) {
-	*pcap = pcap_open_live(iface, RTE_ETH_PCAP_SNAPLEN,
+open_iface_live(const char *iface, osdep_pcap **pcap) {
+	*pcap = osdep_pcap_open_live(iface, RTE_ETH_PCAP_SNAPLEN,
 			RTE_ETH_PCAP_PROMISC, RTE_ETH_PCAP_TIMEOUT, errbuf);
 
 	if (*pcap == NULL) {
@@ -459,7 +459,7 @@  open_iface_live(const char *iface, pcap_t **pcap) {
 }
 
 static int
-open_single_iface(const char *iface, pcap_t **pcap)
+open_single_iface(const char *iface, osdep_pcap **pcap)
 {
 	if (open_iface_live(iface, pcap) < 0) {
 		PMD_LOG(ERR, "Couldn't open interface %s", iface);
@@ -470,39 +470,39 @@  open_single_iface(const char *iface, pcap_t **pcap)
 }
 
 static int
-open_single_tx_pcap(const char *pcap_filename, pcap_dumper_t **dumper)
+open_single_tx_pcap(const char *pcap_filename, osdep_pcap_dumper **dumper)
 {
-	pcap_t *tx_pcap;
+	osdep_pcap *tx_pcap;
 
 	/*
-	 * We need to create a dummy empty pcap_t to use it
-	 * with pcap_dump_open(). We create big enough an Ethernet
+	 * We need to create a dummy empty osdep_pcap to use it
+	 * with osdep_pcap_dump_open(). We create big enough an Ethernet
 	 * pcap holder.
 	 */
-	tx_pcap = pcap_open_dead_with_tstamp_precision(DLT_EN10MB,
-			RTE_ETH_PCAP_SNAPSHOT_LEN, PCAP_TSTAMP_PRECISION_NANO);
+	tx_pcap = osdep_pcap_open_dead_with_tstamp_precision(OSDEP_DLT_EN10MB,
+		RTE_ETH_PCAP_SNAPSHOT_LEN, OSDEP_PCAP_TSTAMP_PRECISION_NANO);
 	if (tx_pcap == NULL) {
 		PMD_LOG(ERR, "Couldn't create dead pcap");
 		return -1;
 	}
 
-	/* The dumper is created using the previous pcap_t reference */
-	*dumper = pcap_dump_open(tx_pcap, pcap_filename);
+	/* The dumper is created using the previous osdep_pcap reference */
+	*dumper = osdep_pcap_dump_open(tx_pcap, pcap_filename);
 	if (*dumper == NULL) {
-		pcap_close(tx_pcap);
+		osdep_pcap_close(tx_pcap);
 		PMD_LOG(ERR, "Couldn't open %s for writing.",
 			pcap_filename);
 		return -1;
 	}
 
-	pcap_close(tx_pcap);
+	osdep_pcap_close(tx_pcap);
 	return 0;
 }
 
 static int
-open_single_rx_pcap(const char *pcap_filename, pcap_t **pcap)
+open_single_rx_pcap(const char *pcap_filename, osdep_pcap **pcap)
 {
-	*pcap = pcap_open_offline(pcap_filename, errbuf);
+	*pcap = osdep_pcap_open_offline(pcap_filename, errbuf);
 	if (*pcap == NULL) {
 		PMD_LOG(ERR, "Couldn't open %s: %s", pcap_filename,
 			errbuf);
@@ -513,17 +513,17 @@  open_single_rx_pcap(const char *pcap_filename, pcap_t **pcap)
 }
 
 static uint64_t
-count_packets_in_pcap(pcap_t **pcap, struct pcap_rx_queue *pcap_q)
+count_packets_in_pcap(osdep_pcap **pcap, struct pcap_rx_queue *pcap_q)
 {
-	const u_char *packet;
-	struct pcap_pkthdr header;
+	const uint8_t *packet;
+	struct osdep_pcap_pkthdr header;
 	uint64_t pcap_pkt_count = 0;
 
-	while ((packet = pcap_next(*pcap, &header)))
+	while ((packet = osdep_pcap_next(*pcap, &header)))
 		pcap_pkt_count++;
 
 	/* The pcap is reopened so it can be used as normal later. */
-	pcap_close(*pcap);
+	osdep_pcap_close(*pcap);
 	*pcap = NULL;
 	open_single_rx_pcap(pcap_q->name, pcap);
 
@@ -612,7 +612,7 @@  eth_dev_stop(struct rte_eth_dev *dev)
 
 	/* Special iface case. Single pcap is open and shared between tx/rx. */
 	if (internals->single_iface) {
-		pcap_close(pp->tx_pcap[0]);
+		osdep_pcap_close(pp->tx_pcap[0]);
 		pp->tx_pcap[0] = NULL;
 		pp->rx_pcap[0] = NULL;
 		goto status_down;
@@ -620,19 +620,19 @@  eth_dev_stop(struct rte_eth_dev *dev)
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
 		if (pp->tx_dumper[i] != NULL) {
-			pcap_dump_close(pp->tx_dumper[i]);
+			osdep_pcap_dump_close(pp->tx_dumper[i]);
 			pp->tx_dumper[i] = NULL;
 		}
 
 		if (pp->tx_pcap[i] != NULL) {
-			pcap_close(pp->tx_pcap[i]);
+			osdep_pcap_close(pp->tx_pcap[i]);
 			pp->tx_pcap[i] = NULL;
 		}
 	}
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		if (pp->rx_pcap[i] != NULL) {
-			pcap_close(pp->rx_pcap[i]);
+			osdep_pcap_close(pp->rx_pcap[i]);
 			pp->rx_pcap[i] = NULL;
 		}
 	}
@@ -804,11 +804,11 @@  eth_rx_queue_setup(struct rte_eth_dev *dev,
 
 	if (internals->infinite_rx) {
 		struct pmd_process_private *pp;
-		char ring_name[NAME_MAX];
+		char ring_name[RTE_RING_NAMESIZE];
 		static uint32_t ring_number;
 		uint64_t pcap_pkt_count = 0;
 		struct rte_mbuf *bufs[1];
-		pcap_t **pcap;
+		osdep_pcap **pcap;
 
 		pp = rte_eth_devices[pcap_q->port_id].process_private;
 		pcap = &pp->rx_pcap[pcap_q->queue_id];
@@ -932,7 +932,7 @@  static const struct eth_dev_ops ops = {
 
 static int
 add_queue(struct pmd_devargs *pmd, const char *name, const char *type,
-		pcap_t *pcap, pcap_dumper_t *dumper)
+		osdep_pcap *pcap, osdep_pcap_dumper *dumper)
 {
 	if (pmd->num_of_queue >= RTE_PMD_PCAP_MAX_QUEUES)
 		return -1;
@@ -955,13 +955,13 @@  open_rx_pcap(const char *key, const char *value, void *extra_args)
 {
 	const char *pcap_filename = value;
 	struct pmd_devargs *rx = extra_args;
-	pcap_t *pcap = NULL;
+	osdep_pcap *pcap = NULL;
 
 	if (open_single_rx_pcap(pcap_filename, &pcap) < 0)
 		return -1;
 
 	if (add_queue(rx, pcap_filename, key, pcap, NULL) < 0) {
-		pcap_close(pcap);
+		osdep_pcap_close(pcap);
 		return -1;
 	}
 
@@ -977,13 +977,13 @@  open_tx_pcap(const char *key, const char *value, void *extra_args)
 {
 	const char *pcap_filename = value;
 	struct pmd_devargs *dumpers = extra_args;
-	pcap_dumper_t *dumper;
+	osdep_pcap_dumper *dumper;
 
 	if (open_single_tx_pcap(pcap_filename, &dumper) < 0)
 		return -1;
 
 	if (add_queue(dumpers, pcap_filename, key, NULL, dumper) < 0) {
-		pcap_dump_close(dumper);
+		osdep_pcap_dump_close(dumper);
 		return -1;
 	}
 
@@ -998,7 +998,7 @@  open_rx_tx_iface(const char *key, const char *value, void *extra_args)
 {
 	const char *iface = value;
 	struct pmd_devargs *tx = extra_args;
-	pcap_t *pcap = NULL;
+	osdep_pcap *pcap = NULL;
 
 	if (open_single_iface(iface, &pcap) < 0)
 		return -1;
@@ -1011,13 +1011,14 @@  open_rx_tx_iface(const char *key, const char *value, void *extra_args)
 }
 
 static inline int
-set_iface_direction(const char *iface, pcap_t *pcap,
-		pcap_direction_t direction)
+set_iface_direction(const char *iface, osdep_pcap *pcap,
+		enum osdep_pcap_direction direction)
 {
-	const char *direction_str = (direction == PCAP_D_IN) ? "IN" : "OUT";
-	if (pcap_setdirection(pcap, direction) < 0) {
+	const char *direction_str =
+		(direction == OSDEP_PCAP_D_IN) ? "IN" : "OUT";
+	if (osdep_pcap_setdirection(pcap, direction) < 0) {
 		PMD_LOG(ERR, "Setting %s pcap direction %s failed - %s\n",
-				iface, direction_str, pcap_geterr(pcap));
+				iface, direction_str, osdep_pcap_geterr(pcap));
 		return -1;
 	}
 	PMD_LOG(INFO, "Setting %s pcap direction %s\n",
@@ -1030,12 +1031,12 @@  open_iface(const char *key, const char *value, void *extra_args)
 {
 	const char *iface = value;
 	struct pmd_devargs *pmd = extra_args;
-	pcap_t *pcap = NULL;
+	osdep_pcap *pcap = NULL;
 
 	if (open_single_iface(iface, &pcap) < 0)
 		return -1;
 	if (add_queue(pmd, iface, key, pcap, NULL) < 0) {
-		pcap_close(pcap);
+		osdep_pcap_close(pcap);
 		return -1;
 	}
 
@@ -1057,7 +1058,7 @@  open_rx_iface(const char *key, const char *value, void *extra_args)
 
 		set_iface_direction(pmd->queue[qid].name,
 				pmd->queue[qid].pcap,
-				PCAP_D_IN);
+				OSDEP_PCAP_D_IN);
 	}
 
 	return 0;
@@ -1311,7 +1312,7 @@  pmd_pcap_probe(struct rte_vdev_device *dev)
 	name = rte_vdev_device_name(dev);
 	PMD_LOG(INFO, "Initializing pmd_pcap for %s", name);
 
-	gettimeofday(&start_time, NULL);
+	rte_time_get_us(&start_time);
 	start_cycles = rte_get_timer_cycles();
 	hz = rte_get_timer_hz();
 
diff --git a/drivers/net/pcap/pcap_osdep.c b/drivers/net/pcap/pcap_osdep.c
new file mode 100644
index 000000000..5cac9bb89
--- /dev/null
+++ b/drivers/net/pcap/pcap_osdep.c
@@ -0,0 +1,122 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 Dmitry Kozlyuk
+ */
+
+#include <pcap.h>
+
+#include <rte_common.h>
+
+#include "pcap_osdep.h"
+
+static inline void
+to_osdep_header(const struct pcap_pkthdr *in, struct osdep_pcap_pkthdr *out)
+{
+	out->ts.sec = in->ts.tv_sec;
+	out->ts.usec = in->ts.tv_usec;
+	out->caplen = in->caplen;
+	out->len = in->len;
+}
+
+static inline void
+to_pcap_header(const struct osdep_pcap_pkthdr *in, struct pcap_pkthdr *out)
+{
+	out->ts.tv_sec = in->ts.sec;
+	out->ts.tv_usec = in->ts.usec;
+	out->caplen = in->caplen;
+	out->len = in->len;
+}
+
+osdep_pcap *
+osdep_pcap_open_live(const char *device, int snaplen,
+	int promisc, int to_ms, char *errbuf)
+{
+	RTE_BUILD_BUG_ON(OSDEP_PCAP_ERRBUF_SIZE != PCAP_ERRBUF_SIZE);
+
+	return (osdep_pcap *)pcap_open_live(device, snaplen, promisc, to_ms,
+		errbuf);
+}
+
+osdep_pcap *
+osdep_pcap_open_offline(const char *fname, char *errbuf)
+{
+	return (osdep_pcap *)pcap_open_offline(fname, errbuf);
+}
+
+osdep_pcap *
+osdep_pcap_open_dead_with_tstamp_precision(int linktype, int snaplen,
+	unsigned int precision)
+{
+	RTE_BUILD_BUG_ON(OSDEP_DLT_EN10MB != DLT_EN10MB);
+	RTE_BUILD_BUG_ON(OSDEP_PCAP_TSTAMP_PRECISION_NANO !=
+			PCAP_TSTAMP_PRECISION_NANO);
+
+	return (osdep_pcap *)pcap_open_dead_with_tstamp_precision(linktype,
+		snaplen, precision);
+}
+
+const uint8_t *
+osdep_pcap_next(osdep_pcap *pcap, struct osdep_pcap_pkthdr *header)
+{
+	const uint8_t *data;
+	struct pcap_pkthdr pkthdr;
+
+	data = pcap_next((pcap_t *)pcap, &pkthdr);
+	to_osdep_header(&pkthdr, header);
+	return data;
+}
+
+int
+osdep_pcap_sendpacket(osdep_pcap *pcap, const uint8_t *buf, int size)
+{
+	return pcap_sendpacket((pcap_t *)pcap, buf, size);
+}
+
+void
+osdep_pcap_close(osdep_pcap *pcap)
+{
+	pcap_close((pcap_t *)pcap);
+}
+
+osdep_pcap_dumper *
+osdep_pcap_dump_open(osdep_pcap *pcap, const char *fname)
+{
+	return (osdep_pcap_dumper *)pcap_dump_open((pcap_t *)pcap, fname);
+}
+
+void
+osdep_pcap_dump(uint8_t *user, const struct osdep_pcap_pkthdr *header,
+	const uint8_t *sp)
+{
+	struct pcap_pkthdr pkthdr;
+
+	to_pcap_header(header, &pkthdr);
+	pcap_dump(user, &pkthdr, sp);
+}
+
+int
+osdep_pcap_dump_flush(osdep_pcap_dumper *p)
+{
+	return pcap_dump_flush((pcap_dumper_t *)p);
+}
+
+void
+osdep_pcap_dump_close(osdep_pcap_dumper *p)
+{
+	pcap_dump_close((pcap_dumper_t *)p);
+}
+
+int
+osdep_pcap_setdirection(osdep_pcap *pcap, enum osdep_pcap_direction dir)
+{
+	RTE_BUILD_BUG_ON((int)OSDEP_PCAP_D_INOUT != (int)PCAP_D_INOUT);
+	RTE_BUILD_BUG_ON((int)OSDEP_PCAP_D_IN != (int)PCAP_D_IN);
+	RTE_BUILD_BUG_ON((int)OSDEP_PCAP_D_OUT != (int)PCAP_D_OUT);
+
+	return pcap_setdirection((pcap_t *)pcap, (pcap_direction_t)dir);
+}
+
+const char *
+osdep_pcap_geterr(osdep_pcap *pcap)
+{
+	return pcap_geterr((pcap_t *)pcap);
+}
diff --git a/drivers/net/pcap/pcap_osdep.h b/drivers/net/pcap/pcap_osdep.h
index 46810d86f..bd00b728a 100644
--- a/drivers/net/pcap/pcap_osdep.h
+++ b/drivers/net/pcap/pcap_osdep.h
@@ -6,6 +6,7 @@ 
 #define _RTE_PCAP_OSDEP_
 
 #include <rte_ether.h>
+#include <rte_time.h>
 
 /*
  * Interface manipulation is always OS-specific.
@@ -14,4 +15,53 @@ 
 int osdep_iface_index_get(const char *name);
 int osdep_iface_mac_get(const char *name, struct rte_ether_addr *mac);
 
+/*
+ * On Windows, libpcap (npcap or WinPcap) exposes Win32 API which clashes
+ * with some DPDK constructs. Trivial libpcap wrappers with "osdep_" prefix
+ * are provided to isolate PMD code from Win32 API.
+ */
+
+#define OSDEP_DLT_EN10MB 1
+
+#define OSDEP_PCAP_ERRBUF_SIZE 256
+
+#define OSDEP_PCAP_TSTAMP_PRECISION_NANO 1
+
+/** Handle for an open packet capture. */
+typedef struct osdep_pcap_type osdep_pcap;
+
+/** Handle for an open packet dump. */
+typedef struct osdep_pcap_dumper_type osdep_pcap_dumper;
+
+struct osdep_pcap_pkthdr {
+	struct rte_time_us ts;
+	uint32_t caplen;
+	uint32_t len;
+};
+
+enum osdep_pcap_direction {
+	OSDEP_PCAP_D_INOUT = 0,
+	OSDEP_PCAP_D_IN,
+	OSDEP_PCAP_D_OUT
+};
+
+osdep_pcap *osdep_pcap_open_live(const char *device, int snaplen,
+	int promisc, int to_ms, char *errbuf);
+osdep_pcap *osdep_pcap_open_offline(const char *fname, char *errbuf);
+osdep_pcap *osdep_pcap_open_dead_with_tstamp_precision(int linktype,
+	int snaplen, unsigned int precision);
+const uint8_t *osdep_pcap_next(osdep_pcap *pcap,
+	struct osdep_pcap_pkthdr *header);
+int osdep_pcap_sendpacket(osdep_pcap *pcap, const uint8_t *buf, int size);
+void osdep_pcap_close(osdep_pcap *pcap);
+
+osdep_pcap_dumper *osdep_pcap_dump_open(osdep_pcap *pcap, const char *fname);
+void osdep_pcap_dump(uint8_t *user, const struct osdep_pcap_pkthdr *header,
+	const uint8_t *sp);
+int osdep_pcap_dump_flush(osdep_pcap_dumper *p);
+void osdep_pcap_dump_close(osdep_pcap_dumper *p);
+
+int osdep_pcap_setdirection(osdep_pcap *pcap, enum osdep_pcap_direction dir);
+const char *osdep_pcap_geterr(osdep_pcap *pcap);
+
 #endif