Hi Tero,
Just a few comments on one of your patches - see inline comments below.
Interesting features btw.
Nico.
--?
Nicol?s Pernas Maradei
On 27 February 2015 at 13:43:14, dev-request at dpdk.org (dev-request at
dpdk.org) wrote:
Message: 5?
Date: Fri, 27 Feb 2015 15:42:38 +0200?
From: Tero Aho <tero....@coriant.com>?
To: <dev at dpdk.org>?
Subject: [dpdk-dev] [PATCH 1/3] pcap: utilize underlying real?
interface properties?
Message-ID: <1425044560-23397-2-git-send-email-tero.aho at coriant.com>?
Content-Type: text/plain?
These changes set pcap interface mac address to the real underlying?
interface address instead of the default one. Also real interface link?
status, speed and duplex are reported when eth_link_update is called?
for the pcap interface.?
Signed-off-by: Tero Aho <tero.aho at coriant.com>?
---?
lib/librte_pmd_pcap/rte_eth_pcap.c | 51 +++++++++++++++++++++++++++++++++++---?
1 file changed, 47 insertions(+), 4 deletions(-)?
diff --git a/lib/librte_pmd_pcap/rte_eth_pcap.c
b/lib/librte_pmd_pcap/rte_eth_pcap.c?
index 5e94930..289af28 100644?
--- a/lib/librte_pmd_pcap/rte_eth_pcap.c?
+++ b/lib/librte_pmd_pcap/rte_eth_pcap.c?
@@ -43,6 +43,11 @@?
#include <rte_dev.h>?
#include <net/if.h>?
+#include <sys/socket.h>?
+#include <sys/ioctl.h>?
+#include <string.h>?
+#include <linux/ethtool.h>?
+#include <linux/sockios.h>?
#include <pcap.h>?
@@ -102,6 +107,8 @@ struct pmd_internals {?
unsigned nb_tx_queues;?
int if_index;?
int single_iface;?
+ const char *if_name;?
+ int if_fd;?
};?
const char *valid_arguments[] = {?
@@ -451,6 +458,26 @@ static int?
eth_link_update(struct rte_eth_dev *dev __rte_unused,?
*dev is being used. Remove __rte_unused
int wait_to_complete __rte_unused)?
{?
+ struct ifreq ifr;?
+ struct ethtool_cmd cmd;?
+ struct pmd_internals *internals = dev->data->dev_private;?
+?
+ if (internals->if_name && (internals->if_fd != -1)) {?
+ /* get link status, speed and duplex from the underlying interface */?
+?
+ strncpy(ifr.ifr_name, internals->if_name, sizeof(ifr.ifr_name)-1);?
+ ifr.ifr_name[sizeof(ifr.ifr_name)-1] = 0;?
Use snprintf(ifr.ifr_name, sizeof(ifr.ifr_name), ?%s?, internals->if_name)
instead. It?s safer and cleaner.
+ if (!ioctl(internals->if_fd, SIOCGIFFLAGS, &ifr))?
+ dev->data->dev_link.link_status = (ifr.ifr_flags & IFF_UP) ? 1 : 0;?
+?
+ cmd.cmd = ETHTOOL_GSET;?
+ ifr.ifr_data = (void *)&cmd;?
+ if (!ioctl(internals->if_fd, SIOCETHTOOL, &ifr)) {?
+ dev->data->dev_link.link_speed = ethtool_cmd_speed(&cmd);?
+ dev->data->dev_link.link_duplex =?
+ cmd.duplex ? ETH_LINK_FULL_DUPLEX : ETH_LINK_HALF_DUPLEX;?
+ }?
+ }?
return 0;?
}?
@@ -736,11 +763,24 @@ rte_pmd_init_internals(const char *name, const unsigned
nb_rx_queues,?
(*internals)->nb_rx_queues = nb_rx_queues;?
(*internals)->nb_tx_queues = nb_tx_queues;?
- if (pair == NULL)?
+ if (pair == NULL) {?
(*internals)->if_index = 0;?
- else?
+ } else {?
+ /* use real inteface mac addr, save name and fd for eth_link_update */?
(*internals)->if_index = if_nametoindex(pair->value);?
-?
+ (*internals)->if_name = strdup(pair->value);?
+ (*internals)->if_fd = socket(AF_INET, SOCK_DGRAM, 0);?
I see you are using a socket and ioctl calls to get the info you need from the
interface. I?m not a big fan of opening a socket at this point just to get some
parameters of the NIC. I?d rather reading those from sysfs. Is there a reason
why you?d prefer to open a socket?
These would be the files you?d need to open and read to the get the info you
are looking for.?
# cat /sys/class/net/eth0/address
# cat /sys/class/net/eth0/duplex
# cat /sys/class/net/eth0/speed
In my opinion the code would be cleaner doing it this way. DPDK already
manipulates?sysfs in other places too.?
What do you think?
+ if ((*internals)->if_fd != -1) {?
+ struct ifreq ifr;?
+ strncpy(ifr.ifr_name, pair->value, sizeof(ifr.ifr_name)-1);?
+ ifr.ifr_name[sizeof(ifr.ifr_name)-1] = 0;?
Use snprintf() like before.
+ if (!ioctl((*internals)->if_fd, SIOCGIFHWADDR, &ifr)) {?
+ data->mac_addrs = rte_zmalloc_socket(NULL, ETHER_ADDR_LEN, 0, numa_node);?
+ if (data->mac_addrs)?
+ rte_memcpy(data->mac_addrs, ifr.ifr_addr.sa_data, ETHER_ADDR_LEN);?
+ }?
+ }?
+ }?
pci_dev->numa_node = numa_node;?
data->dev_private = *internals;?
@@ -749,7 +789,8 @@ rte_pmd_init_internals(const char *name, const unsigned
nb_rx_queues,?
data->nb_rx_queues = (uint16_t)nb_rx_queues;?
data->nb_tx_queues = (uint16_t)nb_tx_queues;?
data->dev_link = pmd_link;?
- data->mac_addrs = ð_addr;?
+ if (data->mac_addrs == NULL)?
+ data->mac_addrs = ð_addr;?
strncpy(data->name,?
(*eth_dev)->data->name, strlen((*eth_dev)->data->name));?
@@ -758,6 +799,8 @@ rte_pmd_init_internals(const char *name, const unsigned
nb_rx_queues,?
(*eth_dev)->pci_dev = pci_dev;?
(*eth_dev)->driver = &rte_pcap_pmd;?
+ eth_link_update((*eth_dev), 0);?
+?
return 0;?
error: if (data)?
--?
1.9.1?
============================================================?
The information contained in this message may be privileged?
and confidential and protected from disclosure. If the reader?
of this message is not the intended recipient, or an employee?
or agent responsible for delivering this message to the?
intended recipient, you are hereby notified that any reproduction,?
dissemination or distribution of this communication is strictly?
prohibited. If you have received this communication in error,?
please notify us immediately by replying to the message and?
deleting it from your computer. Thank you. Coriant-Tellabs?
============================================================?
------------------------------?
Subject: Digest Footer?
_______________________________________________?
dev mailing list?
dev at dpdk.org?
http://dpdk.org/ml/listinfo/dev?
------------------------------?
End of dev Digest, Vol 29, Issue 99?
***********************************?
