diff mbox series

[2/2] net/ark: remove RTE_LIBRTE_ARK_PAD_TX configuration macro

Message ID 20200824133641.19450-2-ed.czeck@atomicrules.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [1/2] net/ark: remove compile time log macros in favor of run time log control |

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Ed Czeck Aug. 24, 2020, 1:36 p.m. UTC 
  Replace behavior with RTE_LIBRTE_ARK_MIN_TX_PKTLEN
with a default value of 0.
Update documentation as needed.

Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
---
 doc/guides/nics/ark.rst         | 13 ++++++++--
 drivers/net/ark/ark_ethdev_tx.c | 44 +++++++++++++++++++--------------
 drivers/net/ark/ark_logs.h      |  8 ------
 3 files changed, 36 insertions(+), 29 deletions(-)

Comments

Ferruh Yigit Aug. 24, 2020, 2:55 p.m. UTC | #1 
On 8/24/2020 2:36 PM, Ed Czeck wrote:
> Replace behavior with RTE_LIBRTE_ARK_MIN_TX_PKTLEN
> with a default value of 0.
> Update documentation as needed.
> 
> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> ---
>  doc/guides/nics/ark.rst         | 13 ++++++++--
>  drivers/net/ark/ark_ethdev_tx.c | 44 +++++++++++++++++++--------------
>  drivers/net/ark/ark_logs.h      |  8 ------
>  3 files changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
> index c3ffcbbc2..f4d4539e5 100644
> --- a/doc/guides/nics/ark.rst
> +++ b/doc/guides/nics/ark.rst
> @@ -129,8 +129,10 @@ Configuration Information
>     * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
>       of the ARK PMD driver in the DPDK compilation.
>  
> -   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
> -     packets are padded to 60 bytes to support downstream MACS.
> +  * **CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN** (default 0): Sets the minimum
> +    packet length for tx packets to the FPGA.  Packets less than this
> +    length are padded to meet the requirement. This allows padding to
> +    be offloaded or remain in host software.
>  

There is no 'CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN' option, I think it is
confusing to add it to documentation. There is already some documentation in
below for 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN', what do you think removing this part?

And can you please remove 'CONFIG_RTE_LIBRTE_ARK_PAD_TX' form 'config/common_base'?

Another option is, do you think does it help your users to keep the config name
same, like keep the 'CONFIG_RTE_LIBRTE_ARK_PAD_TX' as 'RTE_LIBRTE_ARK_PAD_TX'
for the meson CFLAGs, instead of renaming it to 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN'?
Ed Czeck Aug. 24, 2020, 9:51 p.m. UTC | #2 
On Mon, Aug 24, 2020 at 10:55 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 8/24/2020 2:36 PM, Ed Czeck wrote:
> > Replace behavior with RTE_LIBRTE_ARK_MIN_TX_PKTLEN
> > with a default value of 0.
> > Update documentation as needed.
> >
> > Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
> > ---
> >  doc/guides/nics/ark.rst         | 13 ++++++++--
> >  drivers/net/ark/ark_ethdev_tx.c | 44 +++++++++++++++++++--------------
> >  drivers/net/ark/ark_logs.h      |  8 ------
> >  3 files changed, 36 insertions(+), 29 deletions(-)
> >
> > diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
> > index c3ffcbbc2..f4d4539e5 100644
> > --- a/doc/guides/nics/ark.rst
> > +++ b/doc/guides/nics/ark.rst
> > @@ -129,8 +129,10 @@ Configuration Information
> >     * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
> >       of the ARK PMD driver in the DPDK compilation.
> >
> > -   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
> > -     packets are padded to 60 bytes to support downstream MACS.
> > +  * **CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN** (default 0): Sets the minimum
> > +    packet length for tx packets to the FPGA.  Packets less than this
> > +    length are padded to meet the requirement. This allows padding to
> > +    be offloaded or remain in host software.
> >
>
> There is no 'CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN' option, I think it is
> confusing to add it to documentation. There is already some documentation in
> below for 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN', what do you think removing this part?
>
Should I remove the all references to the CONFIG_RTE_LIBRTE_ARK from
the net/ark section of the guide?  It appears that CONFIG_.* will be
removed along with the makefile build system.

> And can you please remove 'CONFIG_RTE_LIBRTE_ARK_PAD_TX' form 'config/common_base'?
>
I am hesitant to modify config/common_base, since it is slated for
removal and edits will cause merge conflicts.  Are you OK with this?

> Another option is, do you think does it help your users to keep the config name
> same, like keep the 'CONFIG_RTE_LIBRTE_ARK_PAD_TX' as 'RTE_LIBRTE_ARK_PAD_TX'
> for the meson CFLAGs, instead of renaming it to 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN'?
I will drop the CONFIG names and leave the compiler marco documented
in my next patch.

Thx,
Ferruh Yigit Aug. 25, 2020, 7:43 a.m. UTC | #3 
On 8/24/2020 10:51 PM, Ed Czeck wrote:
> On Mon, Aug 24, 2020 at 10:55 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> On 8/24/2020 2:36 PM, Ed Czeck wrote:
>>> Replace behavior with RTE_LIBRTE_ARK_MIN_TX_PKTLEN
>>> with a default value of 0.
>>> Update documentation as needed.
>>>
>>> Signed-off-by: Ed Czeck <ed.czeck@atomicrules.com>
>>> ---
>>>  doc/guides/nics/ark.rst         | 13 ++++++++--
>>>  drivers/net/ark/ark_ethdev_tx.c | 44 +++++++++++++++++++--------------
>>>  drivers/net/ark/ark_logs.h      |  8 ------
>>>  3 files changed, 36 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
>>> index c3ffcbbc2..f4d4539e5 100644
>>> --- a/doc/guides/nics/ark.rst
>>> +++ b/doc/guides/nics/ark.rst
>>> @@ -129,8 +129,10 @@ Configuration Information
>>>     * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
>>>       of the ARK PMD driver in the DPDK compilation.
>>>
>>> -   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
>>> -     packets are padded to 60 bytes to support downstream MACS.
>>> +  * **CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN** (default 0): Sets the minimum
>>> +    packet length for tx packets to the FPGA.  Packets less than this
>>> +    length are padded to meet the requirement. This allows padding to
>>> +    be offloaded or remain in host software.
>>>
>>
>> There is no 'CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN' option, I think it is
>> confusing to add it to documentation. There is already some documentation in
>> below for 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN', what do you think removing this part?
>>
> Should I remove the all references to the CONFIG_RTE_LIBRTE_ARK from
> the net/ark section of the guide?  It appears that CONFIG_.* will be
> removed along with the makefile build system.

As you said it is removed by Ciara's patch, no need to touch it in this patch.

> 
>> And can you please remove 'CONFIG_RTE_LIBRTE_ARK_PAD_TX' form 'config/common_base'?
>>
> I am hesitant to modify config/common_base, since it is slated for
> removal and edits will cause merge conflicts.  Are you OK with this?

OK.

> 
>> Another option is, do you think does it help your users to keep the config name
>> same, like keep the 'CONFIG_RTE_LIBRTE_ARK_PAD_TX' as 'RTE_LIBRTE_ARK_PAD_TX'
>> for the meson CFLAGs, instead of renaming it to 'RTE_LIBRTE_ARK_MIN_TX_PKTLEN'?
> I will drop the CONFIG names and leave the compiler marco documented
> in my next patch.
> 

OK, thanks.
diff mbox series

Patch

diff --git a/doc/guides/nics/ark.rst b/doc/guides/nics/ark.rst
index c3ffcbbc2..f4d4539e5 100644
--- a/doc/guides/nics/ark.rst
+++ b/doc/guides/nics/ark.rst
@@ -129,8 +129,10 @@  Configuration Information
    * **CONFIG_RTE_LIBRTE_ARK_PMD** (default y): Enables or disables inclusion
      of the ARK PMD driver in the DPDK compilation.
 
-   * **CONFIG_RTE_LIBRTE_ARK_PAD_TX** (default y):  When enabled TX
-     packets are padded to 60 bytes to support downstream MACS.
+  * **CONFIG_RTE_LIBRTE_ARK_MIN_TX_PKTLEN** (default 0): Sets the minimum
+    packet length for tx packets to the FPGA.  Packets less than this
+    length are padded to meet the requirement. This allows padding to
+    be offloaded or remain in host software.
 
 
 Building DPDK
@@ -144,6 +146,13 @@  By default the ARK PMD library will be built into the DPDK library.
 For configuring and using UIO and VFIO frameworks, please also refer :ref:`the
 documentation that comes with DPDK suite <linux_gsg>`.
 
+To build with a non-zero minimum tx packet length, set the above macro in your
+CFLAGS environment prior to the meson build step. I.e.,
+
+    export CFLAGS="-DRTE_LIBRTE_ARK_MIN_TX_PKTLEN=60"
+    meson build
+
+
 Supported ARK RTL PCIe Instances
 --------------------------------
 
diff --git a/drivers/net/ark/ark_ethdev_tx.c b/drivers/net/ark/ark_ethdev_tx.c
index 72624deb3..d24d7ac21 100644
--- a/drivers/net/ark/ark_ethdev_tx.c
+++ b/drivers/net/ark/ark_ethdev_tx.c
@@ -15,6 +15,13 @@ 
 #define ARK_TX_MAX_NOCHAIN (RTE_MBUF_DEFAULT_DATAROOM)
 
 
+#ifndef RTE_LIBRTE_ARK_MIN_TX_PKTLEN
+#define ARK_MIN_TX_PKTLEN 0
+#else
+#define ARK_MIN_TX_PKTLEN RTE_LIBRTE_ARK_MIN_TX_PKTLEN
+#endif
+
+
 /* ************************************************************************* */
 struct ark_tx_queue {
 	struct ark_tx_meta *meta_q;
@@ -91,6 +98,7 @@  eth_ark_xmit_pkts(void *vtxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	uint32_t prod_index_limit;
 	int stat;
 	uint16_t nb;
+	const uint32_t min_pkt_len = ARK_MIN_TX_PKTLEN;
 
 	queue = (struct ark_tx_queue *)vtxq;
 
@@ -104,27 +112,25 @@  eth_ark_xmit_pkts(void *vtxq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 	     ++nb) {
 		mbuf = tx_pkts[nb];
 
-		if (ARK_TX_PAD_TO_60) {
-			if (unlikely(rte_pktmbuf_pkt_len(mbuf) < 60)) {
-				/* this packet even if it is small can be split,
-				 * be sure to add to the end mbuf
+		if (unlikely(rte_pktmbuf_pkt_len(mbuf) < min_pkt_len)) {
+			/* this packet even if it is small can be split,
+			 * be sure to add to the end mbuf
+			 */
+			uint16_t to_add = min_pkt_len -
+				rte_pktmbuf_pkt_len(mbuf);
+			char *appended =
+				rte_pktmbuf_append(mbuf, to_add);
+
+			if (appended == 0) {
+				/* This packet is in error,
+				 * we cannot send it so just
+				 * count it and delete it.
 				 */
-				uint16_t to_add =
-					60 - rte_pktmbuf_pkt_len(mbuf);
-				char *appended =
-					rte_pktmbuf_append(mbuf, to_add);
-
-				if (appended == 0) {
-					/* This packet is in error,
-					 * we cannot send it so just
-					 * count it and delete it.
-					 */
-					queue->tx_errors += 1;
-					rte_pktmbuf_free(mbuf);
-					continue;
-				}
-				memset(appended, 0, to_add);
+				queue->tx_errors += 1;
+				rte_pktmbuf_free(mbuf);
+				continue;
 			}
+			memset(appended, 0, to_add);
 		}
 
 		if (unlikely(mbuf->nb_segs != 1)) {
diff --git a/drivers/net/ark/ark_logs.h b/drivers/net/ark/ark_logs.h
index c3d7e7d39..ca46d86c9 100644
--- a/drivers/net/ark/ark_logs.h
+++ b/drivers/net/ark/ark_logs.h
@@ -8,14 +8,6 @@ 
 #include <inttypes.h>
 #include <rte_log.h>
 
-
-/* Configuration option to pad TX packets to 60 bytes */
-#ifdef RTE_LIBRTE_ARK_PAD_TX
-#define ARK_TX_PAD_TO_60   1
-#else
-#define ARK_TX_PAD_TO_60   0
-#endif
-
 /* system camel case definition changed to upper case */
 #define PRIU32 PRIu32
 #define PRIU64 PRIu64