Re: [dpdk-dev] dev Digest, Vol 195, Issue 45

Re: [dpdk-dev] dev Digest, Vol 195, Issue 45

[Gavin Hu]

-----Original Message-----
From: dev <dev-bounces@dpdk.org> On Behalf Of dev-request@dpdk.org
Sent: Tuesday, May 15, 2018 5:12 PM
To: dev@dpdk.org
Subject: dev Digest, Vol 195, Issue 45

Send dev mailing list submissions to
dev@dpdk.org

To subscribe or unsubscribe via the World Wide Web, visit
https://dpdk.org/ml/listinfo/dev
or, via email, send a message with subject or body 'help' to
dev-request@dpdk.org

You can reach the person managing the list at
dev-owner@dpdk.org

When replying, please edit your Subject line so it is more specific than "Re: Contents of dev digest..."


Today's Topics:

   1. Re: [PATCH 1/4] app: add LDFLAGS -latomic to link atomic lib
      (Jerin Jacob)
   2. [PATCH v2] net/tap: perform proto field update for tunonly
      (Vipin Varghese)
   3. Re: [PATCH] net/tap: perform proto field update for tun only
      (Varghese, Vipin)
   4. Re: [PATCH 2/4] Driver/Mellanox: fix PMD compiling issue
      (Jerin Jacob)


----------------------------------------------------------------------

Message: 1
Date: Tue, 15 May 2018 14:37:01 +0530
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: Gavin Hu <gavin.hu@arm.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 1/4] app: add LDFLAGS -latomic to link
atomic lib
Message-ID: <20180515090700.GA10539@jerin>
Content-Type: text/plain; charset=us-ascii

-----Original Message-----
> Date: Tue, 15 May 2018 04:28:41 -0400
> From: Gavin Hu <gavin.hu@arm.com>
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/4] app: add LDFLAGS -latomic to link
> atomic lib
> X-Mailer: git-send-email 2.1.4
>
> For ARM64 platform, libdpdk.a includes the
> librte_pmd_octeontx_ssovf.a, which requires the libatomic.a
> support.The atomic lib is built-in in the gcc toolchain, but for clang it has to be explicitly linked.
> For more details, please refer to
> https://clang.llvm.org/docs/Toolchain.html
>
> ~/dpdk/build/lib/librte_pmd_octeontx_ssovf.a(timvf_worker.o): In function `timvf_timer_cancel_burst':
> timvf_worker.c:(.text+0x80): undefined reference to `__atomic_fetch_add_8'
> /home/gavin/arm_repo/dpdk/build/lib/librte_pmd_octeontx_ssovf.a(timvf_worker.o): In function `timvf_timer_arm_burst_sp':
> timvf_worker.c:(.text+0x200): undefined reference to `__atomic_fetch_add_8'
> timvf_worker.c:(.text+0x244): undefined reference to `__atomic_store_2'
> timvf_worker.c:(.text+0x278): undefined reference to `__atomic_fetch_add_4'
> timvf_worker.c:(.text+0x30c): undefined reference to `__atomic_store_2'
>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>



Following patch is part of upstream. Are you testing with following patch/upstream.

With this patch from Nikhilesh, this __ atomic__ compiling issue was gone.
The two patches fix the same issue.
Should I abandon my patch?

I see this note on: https://clang.llvm.org/docs/Toolchain.html
Note
Clang does not currently automatically link against libatomic when using libgcc_s. You may need to manually add -latomic to support this configuration when using non-native atomic operations (if you see link errors referring to __atomic_* functions).

commit 55fbc92d7800100628579643c9ee2770614fef10
Author: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
Date:   Wed May 9 02:56:00 2018 +0530

    event/octeontx: fix build with clang 6

    Clang 6 & 7 fail to naturally align packed structs due to this clang
    can't use 8byte atomic primitives and splits them into lesser atomic
    primitives. To use lesser atomic primitives we need to link libatomic
    (-latomic), instead supply alignment attribute to the compiler.

    timvf_worker.c:(.text+0x498): undefined reference to `__atomic_fetch_add_8'
    timvf_worker.c:(.text+0x525): undefined reference to `__atomic_store_2'
    timvf_worker.c:(.text+0x557): undefined reference to `__atomic_fetch_add_4'
    timvf_worker.c:(.text+0x5de): undefined reference to `__atomic_store_2'

    Fixes: f874c1eb1519 ("event/octeontx: create and free timer
adapter")

    Reported-by: Andrew Rybchenko <arybchenko@solarflare.com>
    Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
    Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>


> ---
>  mk/rte.app.mk | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk index 438f99d..bca8325
> 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -51,6 +51,7 @@ endif
>
>  # Link only the libraries used in the application  LDFLAGS +=
> --as-needed
> +LDFLAGS += -latomic
>
>  # default path for libs
>  _LDLIBS-y += -L$(RTE_SDK_BIN)/lib
> --
> 2.1.4
>


------------------------------

Message: 2
Date: Tue, 15 May 2018 20:19:40 +0530
From: Vipin Varghese <vipin.varghese@intel.com>
To: dev@dpdk.org, keith.wiles@intel.com, ferruh.yigit@intel.com,
ophirmu@mellanox.com
Cc: Vipin Varghese <vipin.varghese@intel.com>
Subject: [dpdk-dev] [PATCH v2] net/tap: perform proto field update for
tunonly
Message-ID:
<1526395780-105792-1-git-send-email-vipin.varghese@intel.com>

The TX function is shared between TAP and TUN PMD. Checking TUN-TAP
type field will ensure the TAP PMD will always have protocol field
as 0.

Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
Suggested-by: Ferruh Yigit <ferruh.yigit@intel.com>
---

Changes in V2:
updated in comment wording - Keith Wiles
remove debug print from tx code - Keith Wiles
---
 drivers/net/tap/rte_eth_tap.c | 48 ++++++++++++++++++++++++++-----------------
 drivers/net/tap/rte_eth_tap.h | 10 +++++++++
 2 files changed, 39 insertions(+), 19 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index ea6d899..fafd557 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -115,7 +115,8 @@ tun_alloc(struct pmd_internals *pmd)
  * Do not set IFF_NO_PI as packet information header will be needed
  * to check if a received packet has been truncated.
  */
-ifr.ifr_flags = (tap_type) ? IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
+ifr.ifr_flags = (pmd->type == ETH_TUNTAP_TYPE_TAP) ?
+IFF_TAP : IFF_TUN | IFF_POINTOPOINT;
 snprintf(ifr.ifr_name, IFNAMSIZ, "%s", pmd->name);

 TAP_LOG(DEBUG, "ifr_name '%s'", ifr.ifr_name);
@@ -502,20 +503,22 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 if (rte_pktmbuf_pkt_len(mbuf) > max_size)
 break;

-/*
- * TUN and TAP are created with IFF_NO_PI disabled.
- * For TUN PMD this mandatory as fields are used by
- * Kernel tun.c to determine whether its IP or non IP
- * packets.
- *
- * The logic fetches the first byte of data from mbuf.
- * compares whether its v4 or v6. If none matches default
- * value 0x00 is taken for protocol field.
- */
-char *buff_data = rte_pktmbuf_mtod(seg, void *);
-j = (*buff_data & 0xf0);
-pi.proto = (j == 0x40) ? 0x0008 :
-(j == 0x60) ? 0xdd86 : 0x00;
+if (txq->type == ETH_TUNTAP_TYPE_TUN) {
+/*
+ * TUN and TAP are created with IFF_NO_PI disabled.
+ * For TUN PMD this mandatory as fields are used by
+ * Kernel tun.c to determine whether its IP or non IP
+ * packets.
+ *
+ * The logic fetches the first byte of data from mbuf
+ * then compares whether its v4 or v6. If first byte
+ * is 4 or 6, then protocol field is updated.
+ */
+char *buff_data = rte_pktmbuf_mtod(seg, void *);
+j = (*buff_data & 0xf0);
+pi.proto = (j == 0x40) ? 0x0008 :
+(j == 0x60) ? 0xdd86 : 0x00;
+}

 iovecs[0].iov_base = &pi;
 iovecs[0].iov_len = sizeof(pi);
@@ -1052,6 +1055,9 @@ tap_setup_queue(struct rte_eth_dev *dev,
 tx->mtu = &dev->data->mtu;
 rx->rxmode = &dev->data->dev_conf.rxmode;

+tx->type = pmd->type;
+rx->type = pmd->type;
+
 return *fd;
 }

@@ -1386,6 +1392,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 pmd = dev->data->dev_private;
 pmd->dev = dev;
 snprintf(pmd->name, sizeof(pmd->name), "%s", tap_name);
+pmd->type = ETH_TUNTAP_TYPE_UNKNOWN;

 pmd->ioctl_sock = socket(AF_INET, SOCK_DGRAM, 0);
 if (pmd->ioctl_sock == -1) {
@@ -1421,7 +1428,9 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 pmd->txq[i].fd = -1;
 }

-if (tap_type) {
+pmd->type = (tap_type) ? ETH_TUNTAP_TYPE_TAP : ETH_TUNTAP_TYPE_TUN;
+
+if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 if (is_zero_ether_addr(mac_addr))
 eth_random_addr((uint8_t *)&pmd->eth_addr);
 else
@@ -1440,7 +1449,7 @@ eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
 if (tap_ioctl(pmd, SIOCSIFMTU, &ifr, 1, LOCAL_AND_REMOTE) < 0)
 goto error_exit;

-if (tap_type) {
+if (pmd->type == ETH_TUNTAP_TYPE_TAP) {
 memset(&ifr, 0, sizeof(struct ifreq));
 ifr.ifr_hwaddr.sa_family = AF_LOCAL;
 rte_memcpy(ifr.ifr_hwaddr.sa_data, &pmd->eth_addr,
@@ -1812,7 +1821,9 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 struct pmd_internals *internals;
 int i;

-TAP_LOG(DEBUG, "Closing TUN/TAP Ethernet device on numa %u",
+internals = eth_dev->data->dev_private;
+TAP_LOG(DEBUG, "Closing %s Ethernet device on numa %u",
+(internals->type == ETH_TUNTAP_TYPE_TAP) ? "TAP" : "TUN",
 rte_socket_id());

 /* find the ethdev entry */
@@ -1820,7 +1831,6 @@ rte_pmd_tap_remove(struct rte_vdev_device *dev)
 if (!eth_dev)
 return 0;

-internals = eth_dev->data->dev_private;
 if (internals->nlsk_fd) {
 tap_flow_flush(eth_dev, NULL);
 tap_flow_implicit_flush(internals, NULL);
diff --git a/drivers/net/tap/rte_eth_tap.h b/drivers/net/tap/rte_eth_tap.h
index 67c9d4b..8b0da5a 100644
--- a/drivers/net/tap/rte_eth_tap.h
+++ b/drivers/net/tap/rte_eth_tap.h
@@ -23,6 +23,13 @@
 #define RTE_PMD_TAP_MAX_QUEUES1
 #endif

+enum rte_tuntap_type {
+ETH_TUNTAP_TYPE_UNKNOWN,
+ETH_TUNTAP_TYPE_TUN,
+ETH_TUNTAP_TYPE_TAP,
+ETH_TUNTAP_TYPE_MAX,
+};
+
 struct pkt_stats {
 uint64_t opackets;              /* Number of output packets */
 uint64_t ipackets;              /* Number of input packets */
@@ -38,6 +45,7 @@ struct rx_queue {
 uint32_t trigger_seen;          /* Last seen Rx trigger value */
 uint16_t in_port;               /* Port ID */
 int fd;
+int type;  /* Type field - TUN|TAP */
 struct pkt_stats stats;         /* Stats for this RX queue */
 uint16_t nb_rx_desc;            /* max number of mbufs available */
 struct rte_eth_rxmode *rxmode;  /* RX features */
@@ -48,6 +56,7 @@ struct rx_queue {

 struct tx_queue {
 int fd;
+int type;  /* Type field - TUN|TAP */
 uint16_t *mtu;                  /* Pointer to MTU from dev_data */
 uint16_t csum:1;                /* Enable checksum offloading */
 struct pkt_stats stats;         /* Stats for this TX queue */
@@ -57,6 +66,7 @@ struct pmd_internals {
 struct rte_eth_dev *dev;          /* Ethernet device. */
 char remote_iface[RTE_ETH_NAME_MAX_LEN]; /* Remote netdevice name */
 char name[RTE_ETH_NAME_MAX_LEN];  /* Internal Tap device name */
+int type;  /* Type field - TUN|TAP */
 struct ether_addr eth_addr;       /* Mac address of the device port */
 struct ifreq remote_initial_flags;   /* Remote netdevice flags on init */
 int remote_if_index;              /* remote netdevice IF_INDEX */
--
2.7.4



------------------------------

Message: 3
Date: Tue, 15 May 2018 09:08:49 +0000
From: "Varghese, Vipin" <vipin.varghese@intel.com>
To: "Wiles, Keith" <keith.wiles@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "ophirmu@mellanox.com"
<ophirmu@mellanox.com>, "Yigit, Ferruh" <ferruh.yigit@intel.com>
Subject: Re: [dpdk-dev] [PATCH] net/tap: perform proto field update
for tun only
Message-ID:
<4C9E0AB70F954A408CC4ADDBF0F8FA7D4D1EB6B2@BGSMSX101.gar.corp.intel.com>

Content-Type: text/plain; charset="utf-8"

Thanks Keith,

I have made changes and shared v2 patch for both the suggestions.

Thanks
Vipin Varghese

<snipped>

> > +/*
> > + * TUN and TAP are created with IFF_NO_PI disabled.
> > + * For TUN PMD this mandatory as fields are used by
> > + * Kernel tun.c to determine whether its IP or non IP
> > + * packets.
> > + *
> > + * The logic fetches the first byte of data from mbuf.
> > + * compares whether its v4 or v6. If none match default
> > + * value 0x00 is taken for protocol field.
>
> Little reword and remove the ?.? at end of first line.
>
> The logic fetches the first byte of data from mbuf then compares whether it is v4
> or v6. If not equal to zero then it must be a protocol field.
>
>
>
> > + */
> > +char *buff_data = rte_pktmbuf_mtod(seg, void *);
> > +j = (*buff_data & 0xf0);
> > +pi.proto = (j == 0x40) ? 0x0008 :
> > +(j == 0x60) ? 0xdd86 : 0x00;
> > +printf("j %x pi proto %x\n", j, pi.proto);
>
> Should this not be a LOG message or is this debug that was not removed? If log
> message then add move text to explain the output better.
>

<snipped>


------------------------------

Message: 4
Date: Tue, 15 May 2018 14:41:42 +0530
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: Gavin Hu <gavin.hu@arm.com>
Cc: dev@dpdk.org, Sirshak Das <sirshak.das@arm.com>
Subject: Re: [dpdk-dev] [PATCH 2/4] Driver/Mellanox: fix PMD compiling
issue
Message-ID: <20180515091141.GB10539@jerin>
Content-Type: text/plain; charset=us-ascii

-----Original Message-----
> Date: Tue, 15 May 2018 04:28:42 -0400
> From: Gavin Hu <gavin.hu@arm.com>
> To: dev@dpdk.org
> CC: Sirshak Das <sirshak.das@arm.com>
> Subject: [dpdk-dev] [PATCH 2/4] Driver/Mellanox: fix PMD compiling issue
> X-Mailer: git-send-email 2.1.4

Please add the reason and compilation error log.
and make sure to fix check patch.sh and check-gitlog.sh errors.

Sure I will rework and submit a V3 patch. Thanks for your comments.

>
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Signed-off-by: Sirshak Das <sirshak.das@arm.com>
> Reviewed-by: Phil Yang <Phil.Yang@arm.com>
> Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> ---
>  drivers/net/mlx5/mlx5_rxtx_vec_neon.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/mlx5/mlx5_rxtx_vec_neon.h b/drivers/net/mlx5/mlx5_rxtx_vec_neon.h
> index 2673d6b..71a5eaf 100644
> --- a/drivers/net/mlx5/mlx5_rxtx_vec_neon.h
> +++ b/drivers/net/mlx5/mlx5_rxtx_vec_neon.h
> @@ -167,8 +167,8 @@ txq_scatter_v(struct mlx5_txq_data *txq, struct rte_mbuf **pkts,
>  vst1q_u8((void *)t_wqe, ctrl);
>  /* Fill ESEG in the header. */
>  vst1q_u16((void *)(t_wqe + 1),
> -  (uint16x8_t) { 0, 0, cs_flags, rte_cpu_to_be_16(len),
> - 0, 0, 0, 0 });
> +  ((uint16x8_t) { 0, 0, cs_flags, rte_cpu_to_be_16(len),
> +  0, 0, 0, 0 }));
>  txq->wqe_ci = wqe_ci;
>  }
>  if (!n)
> @@ -300,10 +300,10 @@ txq_burst_v(struct mlx5_txq_data *txq, struct rte_mbuf **pkts, uint16_t pkts_n,
>  vst1q_u8((void *)t_wqe, ctrl);
>  /* Fill ESEG in the header. */
>  vst1q_u8((void *)(t_wqe + 1),
> - (uint8x16_t) { 0, 0, 0, 0,
> -cs_flags, 0, 0, 0,
> -0, 0, 0, 0,
> -0, 0, 0, 0 });
> + ((uint8x16_t) { 0, 0, 0, 0,
> + cs_flags, 0, 0, 0,
> + 0, 0, 0, 0,
> + 0, 0, 0, 0 }));
>  #ifdef MLX5_PMD_SOFT_COUNTERS
>  txq->stats.opackets += pkts_n;
>  #endif
> --
> 2.1.4
>


End of dev Digest, Vol 195, Issue 45
************************************
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] dev Digest, Vol 195, Issue 45

[Jerin Jacob]

-----Original Message-----
> Date: Wed, 16 May 2018 02:08:48 +0000
> From: Gavin Hu <Gavin.Hu@arm.com>
> To: "dev@dpdk.org" <dev@dpdk.org>, "Jacob,  Jerin"
>  <Jerin.JacobKollanukkaran@cavium.com>
> Subject: Re: [dpdk-dev] dev Digest, Vol 195, Issue 45
> 
> >
> > For ARM64 platform, libdpdk.a includes the
> > librte_pmd_octeontx_ssovf.a, which requires the libatomic.a
> > support.The atomic lib is built-in in the gcc toolchain, but for clang it has to be explicitly linked.
> > For more details, please refer to
> > https://clang.llvm.org/docs/Toolchain.html
> >
> > ~/dpdk/build/lib/librte_pmd_octeontx_ssovf.a(timvf_worker.o): In function `timvf_timer_cancel_burst':
> > timvf_worker.c:(.text+0x80): undefined reference to `__atomic_fetch_add_8'
> > /home/gavin/arm_repo/dpdk/build/lib/librte_pmd_octeontx_ssovf.a(timvf_worker.o): In function `timvf_timer_arm_burst_sp':
> > timvf_worker.c:(.text+0x200): undefined reference to `__atomic_fetch_add_8'
> > timvf_worker.c:(.text+0x244): undefined reference to `__atomic_store_2'
> > timvf_worker.c:(.text+0x278): undefined reference to `__atomic_fetch_add_4'
> > timvf_worker.c:(.text+0x30c): undefined reference to `__atomic_store_2'
> >
> > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> 
> 
> 
> Following patch is part of upstream. Are you testing with following patch/upstream.
> 
> With this patch from Nikhilesh, this __ atomic__ compiling issue was gone.
> The two patches fix the same issue.
> Should I abandon my patch?

The root cause is analyzed, the -latomic needs only when atomic
operations on variable are not aligned. The fix added to align variable where it was
not correct.This fix the performance issue as well, adding -latomic
will simply hide the problem by not letting to know the
alignment/performance issue. IMO, If there is no dependency with -latomic
in current code base then we should NOT add new dependency.


> 
> I see this note on: https://clang.llvm.org/docs/Toolchain.html
> Note
> Clang does not currently automatically link against libatomic when using libgcc_s. You may need to manually add -latomic to support this configuration when using non-native atomic operations (if you see link errors referring to __atomic_* functions).
> 
> commit 55fbc92d7800100628579643c9ee2770614fef10
> Author: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> Date:   Wed May 9 02:56:00 2018 +0530
> 
>     event/octeontx: fix build with clang 6
> 
>     Clang 6 & 7 fail to naturally align packed structs due to this clang
>     can't use 8byte atomic primitives and splits them into lesser atomic
>     primitives. To use lesser atomic primitives we need to link libatomic
>     (-latomic), instead supply alignment attribute to the compiler.
> 
>     timvf_worker.c:(.text+0x498): undefined reference to `__atomic_fetch_add_8'
>     timvf_worker.c:(.text+0x525): undefined reference to `__atomic_store_2'
>     timvf_worker.c:(.text+0x557): undefined reference to `__atomic_fetch_add_4'
>     timvf_worker.c:(.text+0x5de): undefined reference to `__atomic_store_2'
> 
>     Fixes: f874c1eb1519 ("event/octeontx: create and free timer
> adapter")
> 
>     Reported-by: Andrew Rybchenko <arybchenko@solarflare.com>
>     Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
>     Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>