From patchwork Wed Apr 29 00:30:19 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Nicol=C3=A1s_Pernas_Maradei?= X-Patchwork-Id: 4492 Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 0E64FC734; Wed, 29 Apr 2015 02:31:26 +0200 (CEST) Received: from smtp.digiweb.ie (smtp2.digiweb.ie [83.147.160.14]) by dpdk.org (Postfix) with ESMTP id 9D78AC72C for ; Wed, 29 Apr 2015 02:31:24 +0200 (CEST) Received: from statler.emutex.com (unknown [92.51.199.138]) by smtp.digiweb.ie (Postfix) with ESMTP id E118B290030; Wed, 29 Apr 2015 01:30:20 +0100 (IST) Received: from 188-141-82-118.dynamic.upc.ie ([188.141.82.118] helo=Nicos-MacBook-Pro.local) by statler.emutex.com with esmtpsa (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.72) (envelope-from ) id 1YnFtM-0004B0-2I; Wed, 29 Apr 2015 01:30:20 +0100 Date: Wed, 29 Apr 2015 01:30:19 +0100 From: =?utf-8?Q?Nicol=C3=A1s_Pernas_Maradei?= To: dev@dpdk.org, tero.aho@coriant.com Message-ID: X-Mailer: Airmail (286) MIME-Version: 1.0 Content-Disposition: inline X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH 1/3] pcap: utilize underlying real interface properties X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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@dpdk.org (dev-request@dpdk.org) wrote: Message: 5  Date: Fri, 27 Feb 2015 15:42:38 +0200  From: Tero Aho   To:   Subject: [dpdk-dev] [PATCH 1/3] pcap: utilize underlying real  interface properties  Message-ID: <1425044560-23397-2-git-send-email-tero.aho@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   ---  lib/librte_pmd_pcap/rte_eth_pcap.c | 51 +++++++++++++++++++++++++++++++++++---  1 file changed, 47 insertions(+), 4 deletions(-)  + 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@dpdk.org  http://dpdk.org/ml/listinfo/dev  ------------------------------  End of dev Digest, Vol 29, Issue 99  *********************************** 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   #include   +#include   +#include   +#include   +#include   +#include   #include   @@ -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 =