From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 62E1846066; Tue, 14 Jan 2025 19:38:58 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0A89B40299; Tue, 14 Jan 2025 19:38:58 +0100 (CET) Received: from mail-pl1-f178.google.com (mail-pl1-f178.google.com [209.85.214.178]) by mails.dpdk.org (Postfix) with ESMTP id CA2C840298 for ; Tue, 14 Jan 2025 19:38:56 +0100 (CET) Received: by mail-pl1-f178.google.com with SMTP id d9443c01a7336-21649a7bcdcso100286065ad.1 for ; Tue, 14 Jan 2025 10:38:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1736879936; x=1737484736; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=UokH2sFC7ASxKAO96RMpmlKd22dclRbY59BMKsmMS2w=; b=AF7sy7MXa1YC6OUCPXs5FodJXUhDos8JAyDT37asO0N8+a+Wwk/ZnswvhfXUwJWDEp ihFx4pXYCvE40aQnL+py3X6PsUg2DGUKjU9fjvi0PGJqx7KnVtqQ9Ff9pjTpD6kV/jXZ vqFcl+2fJiS4hPli1WlhQfr6PAcB/040HKQZEnWtXaaPHtyZYve4ocurTgVhSnOUtrGT +NYvpKUx4nzTLXgtBfxPXpQiK1M73Lvi7rEWO/EOkC8X7UP6sFXmf3V9vZ5xJTXBcp9N s595VTUlEE2CzaK9Qjnv1zTTg8Mq4fU/I6pOEaXHc0pH5gwQ1Ly8knnvFJ35T+ZwxxD6 kXqg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736879936; x=1737484736; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=UokH2sFC7ASxKAO96RMpmlKd22dclRbY59BMKsmMS2w=; b=wO7YZVgyICnUnxmeT47aMh6rGDzCerRXG/VrR6YsblFRy4cW9ghIcBG4zuArwGas1v Stll1Fsp19vG581JEjbnta0ChfwmDtYgqvux8lobPQ1duAaK5jZueasLvb3e6pb7ZUnt mUTwTYeXUsHKoYGRhq0g3Y+4F5ZhrvMO2Y0ThwIdGRj1bp3g2Pg/+019hgriJBNxT3ku UPu6+WKVjlr8EuNZCQe6iJM9dfXNCbLZ4k/MG+r5KctfykelbhOhQLtwh/mqFuWLkqd+ MrVHEHb6fcX0qE97IzIfri4XjLDp71jglcgDnBySAA05l9PVSkBc7rBJhqzjZvsTRJTQ okBg== X-Forwarded-Encrypted: i=1; AJvYcCV9ftLFCPzzKMnHKpP6cKYatM7uwbS1IpuI8qBCmP31JSLAODuVmJjR4PRWUT/qs1PEM1U=@dpdk.org X-Gm-Message-State: AOJu0Yz3tVgiBow9wp5+ZrKWJcJR/udGPpIK0fM+yJ4o2SsacrmLyPfE m/WvbpEgY4IlZYjwp3EMUI+G7F3CqHQ9fXrqkAcaTlPK/uzVOm7uh64kPoav6bfBx35eVMKo6WI y X-Gm-Gg: ASbGncsA/h++H/ChA2to1GVky2Oen1lDePNJZ1W1XHIByDCFqIaGAShe4lLBpGPwscL lWynRfmxTc5zIkvRMaWEaPDNYqxtV9JCF++rGYHHdOCEbLOJrgY7phSWJdOUHZCFtinqWXQRrMZ EuVXiNkFG6f9l3jgsGj4gso+Okl52j/JPwa14aN/xN/PW+RiI2Aga28PBfgPlh/JziRZm632TYN AecV/QhmACfJmijWdjBW35ydNjlprkxQ3aZPqE0kY6waMXhJFrI5t5y1QSw4XoLhtfxEoKsSKLG OIBIFq4kiKWqZuzGu2UdlgJ2/EB6ff8tzg== X-Google-Smtp-Source: AGHT+IGeAnCrMRfpQ194J4pOBfgzqVAH3UTZoVs/m/GUR5+hnFmgkXoengE3J14YAXEKkxxqVsAxiw== X-Received: by 2002:a17:903:18b:b0:215:577b:ab77 with SMTP id d9443c01a7336-21a83fc0197mr374544565ad.39.1736879935712; Tue, 14 Jan 2025 10:38:55 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-21a9f10f97csm70873145ad.16.2025.01.14.10.38.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 14 Jan 2025 10:38:55 -0800 (PST) Date: Tue, 14 Jan 2025 10:38:53 -0800 From: Stephen Hemminger To: Tudor Cornea Cc: linville@tuxdriver.com, ferruh.yigit@amd.com, andrew.rybchenko@oktetlabs.ru, dev@dpdk.org Subject: Re: [PATCH v2] net/af_packet: allow changing fanout mode Message-ID: <20250114103853.330cd3b6@hermes.local> In-Reply-To: <20250106153508.11262-1-tudor.cornea@gmail.com> References: <20241212080442.1628366-1-tudor.cornea@gmail.com> <20250106153508.11262-1-tudor.cornea@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, 6 Jan 2025 17:35:08 +0200 Tudor Cornea wrote: > This allows us to control the algorithm used to spread traffic between > sockets, adding more fine grained control. If the user does not > specify a fanout mode, the PMD driver will default to > PACKET_FANOUT_HASH. > > Signed-off-by: Tudor Cornea > > --- > v2: > * Renamed the patch > * Replaced packet_fanout argument with fanout_mode, which allows > more fine grained control > --- > doc/guides/nics/af_packet.rst | 4 +- > drivers/net/af_packet/rte_eth_af_packet.c | 92 ++++++++++++++++++++--- > 2 files changed, 83 insertions(+), 13 deletions(-) > > diff --git a/doc/guides/nics/af_packet.rst b/doc/guides/nics/af_packet.rst > index a343d3a961..3443f95004 100644 > --- a/doc/guides/nics/af_packet.rst > +++ b/doc/guides/nics/af_packet.rst > @@ -25,6 +25,8 @@ Some of these, in turn, will be used to configure the PACKET_MMAP settings. > * ``qpairs`` - number of Rx and Tx queues (optional, default 1); > * ``qdisc_bypass`` - set PACKET_QDISC_BYPASS option in AF_PACKET (optional, > disabled by default); > +* ``fanout_mode`` - set fanout algorithm. Possible choices: hash,lb,cpu,rollover,rnd,qm (optional, > + default hash); Use proper punctuation here, need space after comma. A description of what the differences are or reference to af-packet man page would help. https://www.man7.org/linux/man-pages/man7/packet.7.html The user should not have to huting to find what "qm" means for example. Should also address the earlier part in this document that is: The PACKET_FANOUT_HASH behavior of AF_PACKET is used for frame reception. > * ``blocksz`` - PACKET_MMAP block size (optional, default 4096); > * ``framesz`` - PACKET_MMAP frame size (optional, default 2048B; Note: multiple > of 16B); > @@ -64,7 +66,7 @@ framecnt=512): > > .. code-block:: console > > - --vdev=eth_af_packet0,iface=tap0,blocksz=4096,framesz=2048,framecnt=512,qpairs=1,qdisc_bypass=0 > + --vdev=eth_af_packet0,iface=tap0,blocksz=4096,framesz=2048,framecnt=512,qpairs=1,qdisc_bypass=0,fanout_mode=hash > > Features and Limitations > ------------------------ > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c b/drivers/net/af_packet/rte_eth_af_packet.c > index ceb8d9356a..8449975384 100644 > --- a/drivers/net/af_packet/rte_eth_af_packet.c > +++ b/drivers/net/af_packet/rte_eth_af_packet.c > @@ -36,6 +36,7 @@ > #define ETH_AF_PACKET_FRAMESIZE_ARG "framesz" > #define ETH_AF_PACKET_FRAMECOUNT_ARG "framecnt" > #define ETH_AF_PACKET_QDISC_BYPASS_ARG "qdisc_bypass" > +#define ETH_AF_PACKET_FANOUT_MODE_ARG "fanout_mode" > > #define DFLT_FRAME_SIZE (1 << 11) > #define DFLT_FRAME_COUNT (1 << 9) > @@ -96,6 +97,7 @@ static const char *valid_arguments[] = { > ETH_AF_PACKET_FRAMESIZE_ARG, > ETH_AF_PACKET_FRAMECOUNT_ARG, > ETH_AF_PACKET_QDISC_BYPASS_ARG, > + ETH_AF_PACKET_FANOUT_MODE_ARG, > NULL > }; > > @@ -700,6 +702,61 @@ open_packet_iface(const char *key __rte_unused, > return 0; > } > > +#if defined(PACKET_FANOUT) Since PACKET_FANOUT has existed since Linux 3.1, the #ifdef for this can be removed now. > +#define PACKET_FANOUT_INVALID -1 > + > +static int > +get_fanout_group_id(int if_index) > +{ > + return (getpid() ^ if_index) & 0xffff; > +} > + > +static int > +get_fanout_mode(const char *fanout_mode) > +{ > + int mode = PACKET_FANOUT_FLAG_DEFRAG; > + > +#if defined(PACKET_FANOUT_FLAG_ROLLOVER) No more #ifdefs please > + mode |= PACKET_FANOUT_FLAG_ROLLOVER; > +#endif > + > + if (!fanout_mode) { > + /* Default */ > + mode |= PACKET_FANOUT_HASH; > + } else if (!strcmp(fanout_mode, "hash")) { > + mode |= PACKET_FANOUT_HASH; > + } else if (!strcmp(fanout_mode, "lb")) { > + mode |= PACKET_FANOUT_LB; > + } else if (!strcmp(fanout_mode, "cpu")) { > + mode |= PACKET_FANOUT_CPU; > + } else if (!strcmp(fanout_mode, "rollover")) { > + mode |= PACKET_FANOUT_ROLLOVER; > + } else if (!strcmp(fanout_mode, "rnd")) { > + mode |= PACKET_FANOUT_RND; > + } else if (!strcmp(fanout_mode, "qm")) { > + mode |= PACKET_FANOUT_QM; > + } else { > + /* Invalid Fanout Mode */ > + mode = PACKET_FANOUT_INVALID; > + } Maybe allow longer names like "load_balance" or "queue_map" > + > + return mode; > +} > + > +static int > +get_fanout(const char *fanout_mode, int if_index) > +{ > + int group_id = get_fanout_group_id(if_index); > + int mode = get_fanout_mode(fanout_mode); > + int fanout = PACKET_FANOUT_INVALID; > + > + if (mode != PACKET_FANOUT_INVALID) > + fanout = group_id | (mode << 16); > + > + return fanout; > +} This could be simplified as: static int get_fanout(const char *fanout_mode, int if_index) { int mode = get_fanout_mode(fanout_mode); if (mode != PACKET_FANOUT_INVALID) { return get_fanout_group_id(if_index) | (mode << 16); else return PACKOUT_FANOUT_INVALID; } > +#endif > + > static int > rte_pmd_init_internals(struct rte_vdev_device *dev, > const int sockfd, > @@ -709,6 +766,7 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > unsigned int framesize, > unsigned int framecnt, > unsigned int qdisc_bypass, > + const char *fanout_mode, > struct pmd_internals **internals, > struct rte_eth_dev **eth_dev, > struct rte_kvargs *kvlist) > @@ -810,11 +868,12 @@ rte_pmd_init_internals(struct rte_vdev_device *dev, > sockaddr.sll_ifindex = (*internals)->if_index; > > #if defined(PACKET_FANOUT) > - fanout_arg = (getpid() ^ (*internals)->if_index) & 0xffff; > - fanout_arg |= (PACKET_FANOUT_HASH | PACKET_FANOUT_FLAG_DEFRAG) << 16; > -#if defined(PACKET_FANOUT_FLAG_ROLLOVER) > - fanout_arg |= PACKET_FANOUT_FLAG_ROLLOVER << 16; > -#endif > + fanout_arg = get_fanout(fanout_mode, (*internals)->if_index); > + > + if (fanout_arg == PACKET_FANOUT_INVALID) { Looks better without blank line between get_fanout() and the if() but that is totally a matter of personal taste. > + PMD_LOG(ERR, "Invalid fanout mode: %s", fanout_mode); > + goto error; > + }