From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Juhamatti Kuusisaari <Juhamatti.Kuusisaari@coriant.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v5] net/pcap: physical interface MAC address support
Date: Mon, 10 Sep 2018 13:03:52 +0100 [thread overview]
Message-ID: <3b4f9961-3b3c-d374-bdf3-689437c0d104@intel.com> (raw)
In-Reply-To: <20180906165613.25848-1-juhamatti.kuusisaari@coriant.com>
On 9/6/2018 5:56 PM, Juhamatti Kuusisaari wrote:
> Support for PCAP physical interface MAC with phy_mac=1 devarg.
>
> Signed-off-by: Juhamatti Kuusisaari <juhamatti.kuusisaari@coriant.com>
Hi Juhamatti,
Thanks for the patch.
Can you please add a little more details into commit log that why this new
feature is enabled, what is the use case?
Also can you please send new version of the patches as a reply to previous one,
- via "--in-reply-to" git send-email argument. And a history after commit log
what changed in new version. These helps tracing the multiple versions.
> ---
> doc/guides/rel_notes/release_18_11.rst | 4 +
Please update doc/guides/nics/pcap_ring.rst to document new devargs.
> drivers/net/pcap/rte_eth_pcap.c | 119 +++++++++++++++++++++++--
> 2 files changed, 118 insertions(+), 5 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_18_11.rst b/doc/guides/rel_notes/release_18_11.rst
> index 3ae6b3f58..70966740a 100644
> --- a/doc/guides/rel_notes/release_18_11.rst
> +++ b/doc/guides/rel_notes/release_18_11.rst
> @@ -54,6 +54,10 @@ New Features
> Also, make sure to start the actual text at the margin.
> =========================================================
>
> +* **Added a devarg to use PCAP interface physical MAC address.**
> + A new devarg ``phy_mac`` was introduced to allow users to use physical
> + MAC address of the selected PCAP interface.
> +
>
> API Changes
> -----------
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index e8810a171..8917c4c4d 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -7,6 +7,14 @@
> #include <time.h>
>
> #include <net/if.h>
> +#include <sys/socket.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#ifdef __FreeBSD__
> +#include <sys/sysctl.h>
> +#include <net/if_dl.h>
> +#endif
>
> #include <pcap.h>
>
> @@ -17,6 +25,7 @@
> #include <rte_malloc.h>
> #include <rte_mbuf.h>
> #include <rte_bus_vdev.h>
> +#include <rte_string_fns.h>
>
> #define RTE_ETH_PCAP_SNAPSHOT_LEN 65535
> #define RTE_ETH_PCAP_SNAPLEN ETHER_MAX_JUMBO_FRAME_LEN
> @@ -29,6 +38,7 @@
> #define ETH_PCAP_RX_IFACE_IN_ARG "rx_iface_in"
> #define ETH_PCAP_TX_IFACE_ARG "tx_iface"
> #define ETH_PCAP_IFACE_ARG "iface"
> +#define ETH_PCAP_PHY_MAC_ARG "phy_mac"
>
> #define ETH_PCAP_ARG_MAXLEN 64
>
> @@ -87,6 +97,7 @@ static const char *valid_arguments[] = {
> ETH_PCAP_RX_IFACE_IN_ARG,
> ETH_PCAP_TX_IFACE_ARG,
> ETH_PCAP_IFACE_ARG,
> + ETH_PCAP_PHY_MAC_ARG,
> NULL
> };
>
> @@ -904,12 +915,79 @@ pmd_init_internals(struct rte_vdev_device *vdev,
> return 0;
> }
>
> +static void eth_pcap_update_mac(const char *if_name, struct rte_eth_dev **eth_dev,
> + const unsigned int numa_node)
Please follow convention, to have return type in a separate line above.
> +{
> + void *mac_addrs;
> + PMD_LOG(INFO, "Setting phy MAC for %s\n",
> + if_name);
This can be single line. And please leave an empty line between variable
declaration and code.
> +#ifndef __FreeBSD__
> + int if_fd = socket(AF_INET, SOCK_DGRAM, 0);
> + if (if_fd != -1) {
To reduce the indentation of rest of the code, it helps to reverse the logic,
exit on failure and continue without indentation.
> + struct ifreq ifr;
> + strlcpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name));
> + if (!ioctl(if_fd, SIOCGIFHWADDR, &ifr)) {
> + mac_addrs = rte_zmalloc_socket(NULL, ETHER_ADDR_LEN,
> + 0, numa_node);
> + if (mac_addrs) {
> + (*eth_dev)->data->mac_addrs = mac_addrs;
> + rte_memcpy((*eth_dev)->data->mac_addrs,
> + ifr.ifr_addr.sa_data,
> + ETHER_ADDR_LEN);
> + }
> + }
> + close(if_fd);
> + }
> +#else
There is an assumption that it is either Linux or FreeBSD, that is true for now
but Windows is coming :) Better to be safe here and do an "else if" for FreeBSD.
> + int mib[6];
> + size_t len = 0;
> + char *buf = NULL;
> +
> + mib[0] = CTL_NET;
> + mib[1] = AF_ROUTE;
> + mib[2] = 0;
> + mib[3] = AF_LINK;
> + mib[4] = NET_RT_IFLIST;
> + mib[5] = if_nametoindex(if_name);
> +
> + if (sysctl(mib, 6, NULL, &len, NULL, 0) < 0)
> + return;
> +
> + if (len > 0) {
> + struct if_msghdr *ifm;
> + struct sockaddr_dl *sdl;
> +
> + buf = rte_zmalloc_socket(NULL, len,
> + 0, numa_node);
This is an interim buffer, no need to use rte_zmalloc_socket API, rte_malloc or
regular malloc will do the same.
Also no need to break the line.
> + if (buf) {
> + if (sysctl(mib, 6, buf, &len, NULL, 0) < 0) {
> + rte_free(buf);
> + return;
> + }
> +
> + ifm = (struct if_msghdr *)buf;
> + sdl = (struct sockaddr_dl *)(ifm + 1);
> + mac_addrs = rte_zmalloc_socket(NULL, ETHER_ADDR_LEN,
> + 0, numa_node);
> + if (mac_addrs) {
> + (*eth_dev)->data->mac_addrs = mac_addrs;
Just as a heads up, right now the mac_addrs points to global variable so no need
to worry about it, but please beware of a patch does unique mac address for each
interface.
For that case will need to free the mac_addrs before this assignment, based on
which patch goes in, you may need to update this part.
Patch https://patches.dpdk.org/patch/44170/
> + rte_memcpy((*eth_dev)->data->mac_addrs,
> + LLADDR(sdl),
> + ETHER_ADDR_LEN);
> + }
> + }
> + }
> + if (buf)
> + rte_free(buf);
> +#endif
> +}
> +
> static int
> eth_from_pcaps_common(struct rte_vdev_device *vdev,
> struct pmd_devargs *rx_queues, const unsigned int nb_rx_queues,
> struct pmd_devargs *tx_queues, const unsigned int nb_tx_queues,
> struct rte_kvargs *kvlist, struct pmd_internals **internals,
> - struct rte_eth_dev **eth_dev)
> + const int phy_mac, struct rte_eth_dev **eth_dev)
> {
> struct rte_kvargs_pair *pair = NULL;
> unsigned int k_idx;
> @@ -955,6 +1033,9 @@ eth_from_pcaps_common(struct rte_vdev_device *vdev,
> else
> (*internals)->if_index = if_nametoindex(pair->value);
>
> + if (phy_mac && pair)
> + eth_pcap_update_mac(pair->value, eth_dev, vdev->device.numa_node);
This is a little misleading.
pair will be set only for ETH_PCAP_IFACE_ARG ("iface") argument, it won't work
with rx_iface[_in]/tx_iface arguments. Like below two are same:
1) iface=lo
2) rx_iface=lo,tx_iface=lo
But this code will work only with 1) not 2), better to mention in document.
Also pcap supports multiple queue by repeating devarg, like:
rx_iface=eth0,rx_iface=eth1,rx_iface=eth2,tx_pcap=tx.pcap
This will create tree Rx queues, one for each interface.
Right now "iface" argument is hardcoded to single queue, but without a good
reason, when it changes in the future to support multiple queue, ethdev will use
the MAC from first one, better to clarify in the doc update, or perhaps in the
code comment too.
> +
> return 0;
> }
>
> @@ -962,7 +1043,7 @@ static int
> eth_from_pcaps(struct rte_vdev_device *vdev,
> struct pmd_devargs *rx_queues, const unsigned int nb_rx_queues,
> struct pmd_devargs *tx_queues, const unsigned int nb_tx_queues,
> - struct rte_kvargs *kvlist, int single_iface,
> + struct rte_kvargs *kvlist, int single_iface, int phy_mac,
> unsigned int using_dumpers)
> {
> struct pmd_internals *internals = NULL;
> @@ -970,7 +1051,7 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
> int ret;
>
> ret = eth_from_pcaps_common(vdev, rx_queues, nb_rx_queues,
> - tx_queues, nb_tx_queues, kvlist, &internals, ð_dev);
> + tx_queues, nb_tx_queues, kvlist, &internals, phy_mac, ð_dev);
Please fix ./devtools/checkpatches.sh warnings.
>
> if (ret < 0)
> return ret;
> @@ -989,6 +1070,22 @@ eth_from_pcaps(struct rte_vdev_device *vdev,
> return 0;
> }
>
> +static int
> +select_phy_mac(const char *key, const char *value, void *extra_args)
> +{
> + if (extra_args && strcmp(key, ETH_PCAP_PHY_MAC_ARG) == 0) {
no need strcmp, it has been done by rte_kvargs_process().
> + const int phy_mac = atoi(value);
> + int *enable_phy_mac = extra_args;
> +
> + if (phy_mac != 0 && phy_mac != 1)
> + PMD_LOG(WARNING, "Value should be 0 or 1, set it as 1!");
According below logic value doesn't matter, which seems OK, what about removing
the log? You should document expected values in pcap doc.
> +
> + if (phy_mac)
> + *enable_phy_mac = 1;
> + }
> + return 0;
> +}
> +
> static int
> pmd_pcap_probe(struct rte_vdev_device *dev)
> {
> @@ -999,6 +1096,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> struct pmd_devargs dumpers = {0};
> struct rte_eth_dev *eth_dev;
> int single_iface = 0;
> + int phy_mac = 0;
> int ret;
>
> name = rte_vdev_device_name(dev);
> @@ -1026,6 +1124,16 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
> if (kvlist == NULL)
> return -1;
>
> + /*
> + * We check whether we want to use phy MAC of pcap interface.
> + */
> + if (rte_kvargs_count(kvlist, ETH_PCAP_PHY_MAC_ARG)) {
> + ret = rte_kvargs_process(kvlist, ETH_PCAP_PHY_MAC_ARG,
> + &select_phy_mac, &phy_mac);
> + if (ret < 0)
> + goto free_kvlist;
> + }
> +
> /*
> * If iface argument is passed we open the NICs and use them for
> * reading / writing
> @@ -1084,7 +1192,7 @@ pmd_pcap_probe(struct rte_vdev_device *dev)
>
> create_eth:
> ret = eth_from_pcaps(dev, &pcaps, pcaps.num_of_queue, &dumpers,
> - dumpers.num_of_queue, kvlist, single_iface, is_tx_pcap);
> + dumpers.num_of_queue, kvlist, single_iface, phy_mac, is_tx_pcap);
>
> free_kvlist:
> rte_kvargs_free(kvlist);
> @@ -1128,7 +1236,8 @@ RTE_PMD_REGISTER_PARAM_STRING(net_pcap,
> ETH_PCAP_RX_IFACE_ARG "=<ifc> "
> ETH_PCAP_RX_IFACE_IN_ARG "=<ifc> "
> ETH_PCAP_TX_IFACE_ARG "=<ifc> "
> - ETH_PCAP_IFACE_ARG "=<ifc>");
> + ETH_PCAP_IFACE_ARG "=<ifc> "
> + ETH_PCAP_PHY_MAC_ARG "=<int>");
>
> RTE_INIT(eth_pcap_init_log)
> {
>
next prev parent reply other threads:[~2018-09-10 12:03 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-06 16:56 Juhamatti Kuusisaari
2018-09-10 12:03 ` Ferruh Yigit [this message]
2018-09-10 17:20 ` Kuusisaari, Juhamatti (Coriant - FI/Espoo)
2018-09-10 16:55 ` [dpdk-dev] [PATCH v6] " Juhamatti Kuusisaari
2018-09-11 15:35 ` Ferruh Yigit
2018-10-01 7:30 ` Kuusisaari, Juhamatti (Coriant - FI/Espoo)
2018-10-05 20:27 ` [dpdk-dev] [PATCH v7] " Ferruh Yigit
2018-10-06 0:49 ` [dpdk-dev] [PATCH v8] " Ferruh Yigit
2018-10-08 12:14 ` Ferruh Yigit
2018-10-09 4:30 ` Kuusisaari, Juhamatti (Infinera - FI/Espoo)
2018-10-09 12:04 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3b4f9961-3b3c-d374-bdf3-689437c0d104@intel.com \
--to=ferruh.yigit@intel.com \
--cc=Juhamatti.Kuusisaari@coriant.com \
--cc=dev@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).