seladb / PcapPlusPlus

PcapPlusPlus is a multiplatform C++ library for capturing, parsing and crafting of network packets. It is designed to be efficient, powerful and easy to use. It provides C++ wrappers for the most popular packet processing engines such as libpcap, Npcap, WinPcap, DPDK, AF_XDP and PF_RING.
https://pcapplusplus.github.io/
The Unlicense
2.75k stars 674 forks source link

Jumbo Frames in DPDK devices #1339

Closed gyl30 closed 8 months ago

gyl30 commented 8 months ago

Hello,

I am currently using the PcapPlusPlus library to wrap DPDK for network packet reception and transmission. However, I've encountered an issue where PcapPlusPlus does not support Jumbo Frames by default. This has resulted in difficulties during usage as the packets are being discarded by the network interface card (NIC).

I noticed that in DpdkDevice::initMemPool, when calling rte_pktmbuf_pool_create, the value of data_room_size is set to RTE_MBUF_DEFAULT_BUF_SIZE. This causes jumbo frames to be discarded by the network card when received. Is it possible to support modifying this value to accommodate jumbo frames, or are there better alternatives?

PcapPlusPlus version 23.09

Operating system information

[root@localhost ~]# cat /proc/version
Linux version 3.10.0-1160.el7.x86_64 (mockbuild@kbuilder.bsys.centos.org) (gcc version 4.8.5 20150623 (Red Hat 4.8.5-44) (GCC) ) #1 SMP Mon Oct 19 16:18:59 UTC 2020
[root@localhost ~]#  cat /etc/redhat-release
CentOS Linux release 7.9.2009 (Core)
[root@localhost ~]# 

compile command

cmake .. -DPCAPPP_BUILD_EXAMPLES=OFF -DPCAPPP_BUILD_TESTS=OFF -DCMAKE_BUILD_TYPE=Release -DPCAPPP_BUILD_TUTORIALS=OFF -DPCAPPP_USE_DPDK=ON
seladb commented 8 months ago

I think it's possible to support a configurable packet size and have a default of RTE_MBUF_DEFAULT_BUF_SIZE.

Please note we probably also need to change this: https://github.com/seladb/PcapPlusPlus/blob/1ff377c5de22caf9907fcc31f3f71ea138346155/Pcap%2B%2B/src/MBufRawPacket.cpp#L38

Would you consider opening a PR with this change?

gyl30 commented 8 months ago

https://github.com/seladb/PcapPlusPlus/blob/1ff377c5de22caf9907fcc31f3f71ea138346155/Pcap%2B%2B/src/MBufRawPacket.cpp#L25-L27

https://github.com/seladb/PcapPlusPlus/blob/1ff377c5de22caf9907fcc31f3f71ea138346155/Pcap%2B%2B/src/MBufRawPacket.cpp#L38

https://github.com/seladb/PcapPlusPlus/blob/1ff377c5de22caf9907fcc31f3f71ea138346155/Pcap%2B%2B/src/DpdkDevice.cpp#L425

https://github.com/seladb/PcapPlusPlus/blob/1ff377c5de22caf9907fcc31f3f71ea138346155/Pcap%2B%2B/src/KniDevice.cpp#L149-L157

As seen here, both MBUF_DATA_SIZE_DEFINE and rte_pktmbuf_pool_create depend on RTE_MBUF_DEFAULT_DATAROOM. Do we modify the values in MBUF_DATA_SIZE_DEFINE and rte_pktmbuf_pool_create when enabling Jumbo Frame using macros? For example, like this.

MBufRawPacket.cpp


#ifndef MBUF_DATA_SIZE_DEFINE                                                                                                                                                        
#ifdef JUMBO_FRAME_SIZE_DEFINE                                                                                                                                                       
#   define MBUF_DATA_SIZE_DEFINE (JUMBO_FRAME_SIZE_DEFINE + RTE_PKTMBUF_HEADROOM)                                                                                                    
#else                                                                                                                                                                                
#   define MBUF_DATA_SIZE_DEFINE RTE_MBUF_DEFAULT_DATAROOM                                                                                                                           
#endif                                                                                                                                                                               
#endi

const int MBufRawPacket::MBUF_DATA_SIZE = MBUF_DATA_SIZE_DEFINE;

DpdkDevice.cpp and KniDevice.cpp

#ifdef JUMBO_FRAME_SIZE_DEFINE
#   define MBUF_DATA_SIZE_DEFINE (JUMBO_FRAME_SIZE_DEFINE + RTE_PKTMBUF_HEADROOM)
#else
#   define MBUF_DATA_SIZE_DEFINE RTE_MBUF_DEFAULT_DATAROOM
#endif
    // create mbuf pool                                                                                                                                                              
    memPool = rte_pktmbuf_pool_create(mempoolName, mBufPoolSize, MEMPOOL_CACHE_SIZE, 0, MBUF_DATA_SIZE_DEFINE, rte_socket_id());  

Such modifications appear quite messy. Perhaps we could redefine MBUF_DATA_SIZE_DEFINE in one place and then use it directly in other places.

tigercosmos commented 8 months ago

If only three places, it should be fine. Personally, I consider it is more appropriate to define the value within the header, yet I'm struggling to identify the suitable header for this purpose.

gyl30 commented 8 months ago

If only three places, it should be fine. Personally, I consider it is more appropriate to define the value within the header, yet I'm struggling to identify the suitable header for this purpose.

The beginning of a bad smell indeed. Yes, we should define it in a header file.

seladb commented 8 months ago

I think that instead of a const value, we can add it to the DpdkDevice constructor as an optional argument (with a default size of RTE_MBUF_DEFAULT_BUF_SIZE) so users can set whatever value they want.

MBufRawPacket::init() takes a DpdkDevice as an argument so we can extract this value from there.

Regarding KniDevice - I wouldn't change anything unless you really need it. KNI has been deprecated in DPDK and we'll soon remove it from PcapPlusPlus code-base

gyl30 commented 8 months ago

I've noticed that we try to minimize the inclusion of DPDK headers in our header files. If we modify the function signature in the header file to add a default value for the function parameter, RTE_MBUF_DEFAULT_BUF_SIZE, it will inevitably require including DPDK headers. I'm not sure if this is the right approach.

seladb commented 8 months ago

I've noticed that we try to minimize the inclusion of DPDK headers in our header files. If we modify the function signature in the header file to add a default value for the function parameter, RTE_MBUF_DEFAULT_BUF_SIZE, it will inevitably require including DPDK headers. I'm not sure if this is the right approach.

Yes, you're right. Maybe the default value can be some magic number (like -1 or something else) which will translate to RTE_MBUF_DEFAULT_BUF_SIZE. We can mention it in the doxygen documentation