Re: [PATCH v2] net/af_packet: make bypass configurable
From: Ferruh Yigit <hidden>
Date: 2017-09-20 13:21:38
On 9/19/2017 10:45 PM, Chas Williams wrote:
From: "Charles (Chas) Williams" <redacted> In certain situations, low speed interfaces, it may be desirable to have the flow control provided by the kernel queueing disciplines.
Out of curiosity, do you have any compression of performance numbers with and without qdisc?
quoted hunk ↗ jump to hunk
Signed-off-by: Chas Williams <redacted> --- drivers/net/af_packet/rte_eth_af_packet.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c index 7aa26e5..e3858fa 100644 --- a/drivers/net/af_packet/rte_eth_af_packet.c +++ b/drivers/net/af_packet/rte_eth_af_packet.c@@ -59,6 +59,7 @@ #define ETH_AF_PACKET_BLOCKSIZE_ARG "blocksz" #define ETH_AF_PACKET_FRAMESIZE_ARG "framesz" #define ETH_AF_PACKET_FRAMECOUNT_ARG "framecnt" +#define ETH_AF_PACKET_BYPASS_ARG "bypass"
I thinks argument name "bypass" on its own is not clear what is bypassed, would you mind using something like "qdisc_bypass"?
quoted hunk ↗ jump to hunk
#define DFLT_BLOCK_SIZE (1 << 12) #define DFLT_FRAME_SIZE (1 << 11)@@ -115,6 +116,7 @@ static const char *valid_arguments[] = { ETH_AF_PACKET_BLOCKSIZE_ARG, ETH_AF_PACKET_FRAMESIZE_ARG, ETH_AF_PACKET_FRAMECOUNT_ARG, + ETH_AF_PACKET_BYPASS_ARG,
Same comment here, what about "ETH_AF_PACKET_QDISC_BYPASS_ARG" ?
quoted hunk ↗ jump to hunk
NULL };@@ -559,6 +561,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, unsigned int blockcnt, unsigned int framesize, unsigned int framecnt, + unsigned int bypass, struct pmd_internals **internals, struct rte_eth_dev **eth_dev, struct rte_kvargs *kvlist)@@ -580,9 +583,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, #if defined(PACKET_FANOUT) int fanout_arg; #endif -#if defined(PACKET_QDISC_BYPASS) - int bypass; -#endif for (k_idx = 0; k_idx < kvlist->count; k_idx++) { pair = &kvlist->pairs[k_idx];@@ -698,7 +698,6 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, } #if defined(PACKET_QDISC_BYPASS) - bypass = 1; rc = setsockopt(qsockfd, SOL_PACKET, PACKET_QDISC_BYPASS, &bypass, sizeof(bypass)); if (rc == -1) {@@ -851,6 +850,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, unsigned int framesize = DFLT_FRAME_SIZE; unsigned int framecount = DFLT_FRAME_COUNT; unsigned int qpairs = 1; + unsigned int bypass = 1; /* do some parameter checking */ if (*sockfd < 0)@@ -902,6 +902,16 @@ rte_eth_from_packet(struct rte_vdev_device *dev, } continue; } + if (strstr(pair->key, ETH_AF_PACKET_BYPASS_ARG) != NULL) { + bypass = atoi(pair->value); + if (bypass > 2) {
This accepts "2" too . As far as I can see kernel side checks if this value is zero or not, so perhaps this check is not required at all. But if you want to keep it I guess intention is to limit the value to "0" and "1".
quoted hunk ↗ jump to hunk
+ RTE_LOG(ERR, PMD, + "%s: invalid bypass value\n", + name); + return -1; + } + continue; + } } if (framesize > blocksize) {@@ -927,6 +937,7 @@ rte_eth_from_packet(struct rte_vdev_device *dev, if (rte_pmd_init_internals(dev, *sockfd, qpairs, blocksize, blockcount, framesize, framecount, + bypass, &internals, ð_dev, kvlist) < 0) return -1;@@ -1021,4 +1032,5 @@ RTE_PMD_REGISTER_PARAM_STRING(net_af_packet, "qpairs=<int> " "blocksz=<int> " "framesz=<int> " - "framecnt=<int>"); + "framecnt=<int> " + "bypass=<int>"
Although storage is integer, does is make sense to document it as "0|1" to clarify the usage? );