[dpdk-dev] [PATCH 0/2] net/tap: remove useless offload setup

[dpdk-dev] [PATCH 0/2] net/tap: remove useless offload setup

[Stephen Hemminger]

This looks like some infrastructure that was set for some future
change that never happened. Remove it.

Stephen Hemminger (2):
  tap: remove useless offload capa functions
  tap: replace offload_capa function with define

 drivers/net/tap/rte_eth_tap.c | 58 +++++++++--------------------------
 1 file changed, 15 insertions(+), 43 deletions(-)

-- 
2.30.2

[dpdk-dev] [PATCH 1/2] net/tap: remove useless offload capa functions

[Stephen Hemminger]

Since these always return 0, they were doing nothing useful.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5735988e7c82..aeae6e8f5e93 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -373,15 +373,6 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 	}
 }
 
-static uint64_t
-tap_rx_offload_get_port_capa(void)
-{
-	/*
-	 * No specific port Rx offload capabilities.
-	 */
-	return 0;
-}
-
 static uint64_t
 tap_rx_offload_get_queue_capa(void)
 {
@@ -506,15 +497,6 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	return num_rx;
 }
 
-static uint64_t
-tap_tx_offload_get_port_capa(void)
-{
-	/*
-	 * No specific port Tx offload capabilities.
-	 */
-	return 0;
-}
-
 static uint64_t
 tap_tx_offload_get_queue_capa(void)
 {
@@ -1016,11 +998,9 @@ tap_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->min_rx_bufsize = 0;
 	dev_info->speed_capa = tap_dev_speed_capa();
 	dev_info->rx_queue_offload_capa = tap_rx_offload_get_queue_capa();
-	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
-				    dev_info->rx_queue_offload_capa;
+	dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
 	dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
-	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
-				    dev_info->tx_queue_offload_capa;
+	dev_info->tx_offload_capa = dev_info->tx_queue_offload_capa;
 	dev_info->hash_key_size = TAP_RSS_HASH_KEY_SIZE;
 	/*
 	 * limitation: TAP supports all of IP, UDP and TCP hash
-- 
2.30.2

Re: [dpdk-dev] [PATCH 1/2] net/tap: remove useless offload capa functions

[Andrew Rybchenko]

On 6/16/21 7:15 AM, Stephen Hemminger wrote:
> Since these always return 0, they were doing nothing useful.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

I have no strong opinion on the patch, but sometimes even
empty functions with comments add value. So, I see no point
to touch it. So, I'll wait for maintainer reply.

Re: [dpdk-dev] [PATCH 1/2] net/tap: remove useless offload capa functions

[Stephen Hemminger]

On Thu, 1 Jul 2021 17:16:21 +0300
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:

> On 6/16/21 7:15 AM, Stephen Hemminger wrote:
> > Since these always return 0, they were doing nothing useful.
> > 
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
> 
> I have no strong opinion on the patch, but sometimes even
> empty functions with comments add value. So, I see no point
> to touch it. So, I'll wait for maintainer reply.

There are always many ways to write more obscure and technically correct
code. It is best if the code is the smallest correct way.

“I apologize for such a long letter - I didn't have time to write a short one.”
― Mark Twain

Re: [dpdk-dev] [PATCH 1/2] net/tap: remove useless offload capa functions

[Wiles, Keith]

> On Jul 1, 2021, at 12:18 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> On Thu, 1 Jul 2021 17:16:21 +0300
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
> 
>> On 6/16/21 7:15 AM, Stephen Hemminger wrote:
>>> Since these always return 0, they were doing nothing useful.
>>> 
>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
>> 
>> I have no strong opinion on the patch, but sometimes even
>> empty functions with comments add value. So, I see no point
>> to touch it. So, I'll wait for maintainer reply.
> 
> There are always many ways to write more obscure and technically correct
> code. It is best if the code is the smallest correct way.
> 
> “I apologize for such a long letter - I didn't have time to write a short one.”
> ― Mark Twain

I agree with Stephen, the extra functions provide no useful feature or even the comments are not very useful IMO.

Re: [dpdk-dev] [PATCH 1/2] net/tap: remove useless offload capa functions

[Andrew Rybchenko]

On 7/1/21 10:19 PM, Wiles, Keith wrote:
> 
> 
>> On Jul 1, 2021, at 12:18 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>
>> On Thu, 1 Jul 2021 17:16:21 +0300
>> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru> wrote:
>>
>>> On 6/16/21 7:15 AM, Stephen Hemminger wrote:
>>>> Since these always return 0, they were doing nothing useful.
>>>>
>>>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>  
>>>
>>> I have no strong opinion on the patch, but sometimes even
>>> empty functions with comments add value. So, I see no point
>>> to touch it. So, I'll wait for maintainer reply.
>>
>> There are always many ways to write more obscure and technically correct
>> code. It is best if the code is the smallest correct way.
>>
>> “I apologize for such a long letter - I didn't have time to write a short one.”
>> ― Mark Twain
> 
> I agree with Stephen, the extra functions provide no useful feature or even the comments are not very useful IMO.
> 

It is a bit different here. You're changing the code in an
absolutely equivalent way. IMHO complexity of the code
before and after the patch is absolutely the same.
Anyway, it does not matter a lot. If it think it would be
better in a new way - no problem.

@Keith, could you ack the patch series and I'll happily
apply it.

[dpdk-dev] [PATCH 2/2] net/tap: replace offload_capa function with define

[Stephen Hemminger]

Since the offload values are always the same, these can
just be data instead of code.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/tap/rte_eth_tap.c | 34 +++++++++++++---------------------
 1 file changed, 13 insertions(+), 21 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index aeae6e8f5e93..c31739abf967 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -70,6 +70,17 @@
 
 #define TAP_IOV_DEFAULT_MAX 1024
 
+#define TAP_RX_OFFLOAD (DEV_RX_OFFLOAD_SCATTER |	\
+			DEV_RX_OFFLOAD_IPV4_CKSUM |	\
+			DEV_RX_OFFLOAD_UDP_CKSUM |	\
+			DEV_RX_OFFLOAD_TCP_CKSUM)
+
+#define TAP_TX_OFFLOAD (DEV_TX_OFFLOAD_MULTI_SEGS |	\
+			DEV_TX_OFFLOAD_IPV4_CKSUM |	\
+			DEV_TX_OFFLOAD_UDP_CKSUM |	\
+			DEV_TX_OFFLOAD_TCP_CKSUM |	\
+			DEV_TX_OFFLOAD_TCP_TSO)
+
 static int tap_devices_count;
 
 static const char *tuntap_types[ETH_TUNTAP_TYPE_MAX] = {
@@ -373,15 +384,6 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 	}
 }
 
-static uint64_t
-tap_rx_offload_get_queue_capa(void)
-{
-	return DEV_RX_OFFLOAD_SCATTER |
-	       DEV_RX_OFFLOAD_IPV4_CKSUM |
-	       DEV_RX_OFFLOAD_UDP_CKSUM |
-	       DEV_RX_OFFLOAD_TCP_CKSUM;
-}
-
 static void
 tap_rxq_pool_free(struct rte_mbuf *pool)
 {
@@ -497,16 +499,6 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
 	return num_rx;
 }
 
-static uint64_t
-tap_tx_offload_get_queue_capa(void)
-{
-	return DEV_TX_OFFLOAD_MULTI_SEGS |
-	       DEV_TX_OFFLOAD_IPV4_CKSUM |
-	       DEV_TX_OFFLOAD_UDP_CKSUM |
-	       DEV_TX_OFFLOAD_TCP_CKSUM |
-	       DEV_TX_OFFLOAD_TCP_TSO;
-}
-
 /* Finalize l4 checksum calculation */
 static void
 tap_tx_l4_cksum(uint16_t *l4_cksum, uint16_t l4_phdr_cksum,
@@ -997,9 +989,9 @@ tap_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
 	dev_info->min_rx_bufsize = 0;
 	dev_info->speed_capa = tap_dev_speed_capa();
-	dev_info->rx_queue_offload_capa = tap_rx_offload_get_queue_capa();
+	dev_info->rx_queue_offload_capa = TAP_RX_OFFLOAD;
 	dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
-	dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
+	dev_info->tx_queue_offload_capa = TAP_TX_OFFLOAD;
 	dev_info->tx_offload_capa = dev_info->tx_queue_offload_capa;
 	dev_info->hash_key_size = TAP_RSS_HASH_KEY_SIZE;
 	/*
-- 
2.30.2

Re: [dpdk-dev] [PATCH 2/2] net/tap: replace offload_capa function with define

[Andrew Rybchenko]

On 6/16/21 7:15 AM, Stephen Hemminger wrote:
> Since the offload values are always the same, these can
> just be data instead of code.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>

No strong opinion as well, but above reason is not strong
enough to touch the code. So, I'll wait for the maintainer
here as well.

Re: [dpdk-dev] [PATCH 0/2] net/tap: remove useless offload setup

[Wiles, Keith]

> On Jun 15, 2021, at 11:15 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> This looks like some infrastructure that was set for some future
> change that never happened. Remove it.
> 
> Stephen Hemminger (2):
>  tap: remove useless offload capa functions
>  tap: replace offload_capa function with define
> 
> drivers/net/tap/rte_eth_tap.c | 58 +++++++++--------------------------
> 1 file changed, 15 insertions(+), 43 deletions(-)
> 
> -- 
> 2.30.2
> 

Acked-by: Keith Wiles<Keith.wiles@intel.com> for the series.

Re: [dpdk-dev] [PATCH 0/2] net/tap: remove useless offload setup

[Andrew Rybchenko]

On 7/2/21 4:26 PM, Wiles, Keith wrote:
> 
> 
>> On Jun 15, 2021, at 11:15 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
>>
>> This looks like some infrastructure that was set for some future
>> change that never happened. Remove it.
>>
>> Stephen Hemminger (2):
>>  tap: remove useless offload capa functions
>>  tap: replace offload_capa function with define
>>
>> drivers/net/tap/rte_eth_tap.c | 58 +++++++++--------------------------
>> 1 file changed, 15 insertions(+), 43 deletions(-)
>>
>> -- 
>> 2.30.2
>>
> 
> Acked-by: Keith Wiles<Keith.wiles@intel.com> for the series.

Applied, thanks.