net/nfp: add support of UDP fragmentation offload
Checks
Commit Message
Add the support of UDP fragmentation offload feature.
Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
drivers/common/nfp/nfp_common_ctrl.h | 1 +
drivers/net/nfp/nfd3/nfp_nfd3_dp.c | 7 ++++++-
drivers/net/nfp/nfdk/nfp_nfdk_dp.c | 8 +++++---
drivers/net/nfp/nfp_net_common.c | 8 ++++++--
4 files changed, 18 insertions(+), 6 deletions(-)
Comments
On Sat, 17 Feb 2024 09:54:10 +0800
Chaoyong He <chaoyong.he@corigine.com> wrote:
> Add the support of UDP fragmentation offload feature.
>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
> drivers/common/nfp/nfp_common_ctrl.h | 1 +
> drivers/net/nfp/nfd3/nfp_nfd3_dp.c | 7 ++++++-
> drivers/net/nfp/nfdk/nfp_nfdk_dp.c | 8 +++++---
> drivers/net/nfp/nfp_net_common.c | 8 ++++++--
> 4 files changed, 18 insertions(+), 6 deletions(-)
Looks like this depends on earlier patch, it does not apply directly to main branch.
> - if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0)
> + if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> + (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> goto clean_txd;
That is odd indentation style.
Should be:
if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
(ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
goto clean_txd;
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 17 February 2024 17.47
>
> On Sat, 17 Feb 2024 09:54:10 +0800
> Chaoyong He <chaoyong.he@corigine.com> wrote:
>
> > Add the support of UDP fragmentation offload feature.
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> > ---
> > drivers/common/nfp/nfp_common_ctrl.h | 1 +
> > drivers/net/nfp/nfd3/nfp_nfd3_dp.c | 7 ++++++-
> > drivers/net/nfp/nfdk/nfp_nfdk_dp.c | 8 +++++---
> > drivers/net/nfp/nfp_net_common.c | 8 ++++++--
> > 4 files changed, 18 insertions(+), 6 deletions(-)
>
> Looks like this depends on earlier patch, it does not apply directly to
> main branch.
>
> > - if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0)
> > + if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> > + (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> > goto clean_txd;
>
> That is odd indentation style.
Not formally... it follows the official DPDK Coding Style [1].
[1]: https://doc.dpdk.org/guides/contributing/coding_style.html#general
> Should be:
>
> if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> goto clean_txd;
This indentation style is mentioned as an alternative in the guide. But the example in the guide also uses two tabs for a similar long comparison.
Personally, I also prefer the style suggested by Stephen, so we might want to update the Coding Style. ;-)
On Sat, 17 Feb 2024 19:02:30 +0100
Morten Brørup <mb@smartsharesystems.com> wrote:
> Not formally... it follows the official DPDK Coding Style [1].
>
> [1]: https://doc.dpdk.org/guides/contributing/coding_style.html#general
>
> > Should be:
> >
> > if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> > (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> > goto clean_txd;
>
> This indentation style is mentioned as an alternative in the guide. But the example in the guide also uses two tabs for a similar long comparison.
>
> Personally, I also prefer the style suggested by Stephen, so we might want to update the Coding Style. ;-)
The two tabs is an Intel thing, and I prefer the kernel, line up the conditional style.
We really should have a style that can be describe by clang format.
Other projects like VPP have a target that reformats the code and uses one of the clang format
templates.
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, 17 February 2024 19.11
>
> On Sat, 17 Feb 2024 19:02:30 +0100
> Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > Not formally... it follows the official DPDK Coding Style [1].
> >
> > [1]:
> https://doc.dpdk.org/guides/contributing/coding_style.html#general
> >
> > > Should be:
> > >
> > > if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> > > (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> > > goto clean_txd;
> >
> > This indentation style is mentioned as an alternative in the guide.
> But the example in the guide also uses two tabs for a similar long
> comparison.
> >
> > Personally, I also prefer the style suggested by Stephen, so we might
> want to update the Coding Style. ;-)
>
>
> The two tabs is an Intel thing, and I prefer the kernel, line up the
> conditional style.
I prefer 4 space indentation, which is sufficient to notice the indentation. 8 spaces seems overkill to me, and quickly makes the lines too long.
With the editor configured to show tab as 4 spaces, the kernel alignment style ends up with the same indentation for the condition and the code block:
if (a &&
b)
ctr++;
Whereas with the "tab as 4 spaces" editor configuration, the double indentation style clearly separates the continued condition from code block:
if (a &&
b)
ctr++;
On the other hand, complex conditions are easier readable when aligning logically instead of by a fixed number of tabs, e.g.:
if (a |
(b &
(c ^ d)) |
(e ^ f) |
g)
ctr++;
Placing the operators at the beginning also makes the code more readable:
if (a
| (b
& (c ^ d))
| (e ^ f)
| g)
ctr++;
I guess that coding styles are mostly a matter of taste.
I wonder if any research into coding styles has reached any conclusions or recommendations, beyond mixing coding styles is bad for readability.
> We really should have a style that can be describe by clang format.
> Other projects like VPP have a target that reformats the code and uses
> one of the clang format templates.
Automated code formatting seems like a good idea.
On Sun, Feb 18, 2024 at 11:05:35AM +0100, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Saturday, 17 February 2024 19.11
> >
> > On Sat, 17 Feb 2024 19:02:30 +0100
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> >
> > > Not formally... it follows the official DPDK Coding Style [1].
> > >
> > > [1]:
> > https://doc.dpdk.org/guides/contributing/coding_style.html#general
> > >
> > > > Should be:
> > > >
> > > > if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
> > > > (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
> > > > goto clean_txd;
> > >
> > > This indentation style is mentioned as an alternative in the guide.
> > But the example in the guide also uses two tabs for a similar long
> > comparison.
> > >
> > > Personally, I also prefer the style suggested by Stephen, so we might
> > want to update the Coding Style. ;-)
> >
> >
> > The two tabs is an Intel thing, and I prefer the kernel, line up the
> > conditional style.
>
> I prefer 4 space indentation, which is sufficient to notice the indentation. 8 spaces seems overkill to me, and quickly makes the lines too long.
> With the editor configured to show tab as 4 spaces, the kernel alignment style ends up with the same indentation for the condition and the code block:
>
> if (a &&
> b)
> ctr++;
>
> Whereas with the "tab as 4 spaces" editor configuration, the double indentation style clearly separates the continued condition from code block:
>
> if (a &&
> b)
> ctr++;
>
These two above are the main reasons I much prefer the double indent on
continuation, though I'd also add a third one: it means we don't have a mix
of tabs and spaces for indentation.
However, as stated already indent can be a matter of taste, and there will
be some disagreement about it. The existing coding standards document what
was done in the code base when they were written, and I don't think we
should look to change them. It's a bit annoying having 2 standards for
continuation rather than 1, but it's not exactly a free-for-all, and it's
not something that applies to every line, only to a small subset.
> On the other hand, complex conditions are easier readable when aligning logically instead of by a fixed number of tabs, e.g.:
>
> if (a |
> (b &
> (c ^ d)) |
> (e ^ f) |
> g)
> ctr++;
>
Apart from the alignment of the increment at the end, yes, I admit it is a
little more readable. However, I also think that it's still pretty complex
even with the helpers!
> Placing the operators at the beginning also makes the code more readable:
>
> if (a
> | (b
> & (c ^ d))
> | (e ^ f)
> | g)
> ctr++;
>
+1 to this. I think having operators at the beginning of lines is good. It
also makes it visually clearer that the indent is for line continuation.
> I guess that coding styles are mostly a matter of taste.
>
> I wonder if any research into coding styles has reached any conclusions or recommendations, beyond mixing coding styles is bad for readability.
>
> > We really should have a style that can be describe by clang format.
> > Other projects like VPP have a target that reformats the code and uses
> > one of the clang format templates.
>
> Automated code formatting seems like a good idea.
>
Yep. The trouble is that, if you don't do this from the start, the deltas
will be massive, and confusing in our git history.
/Bruce
On 2/19/2024 10:26 AM, Bruce Richardson wrote:
> On Sun, Feb 18, 2024 at 11:05:35AM +0100, Morten Brørup wrote:
>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>>> Sent: Saturday, 17 February 2024 19.11
>>>
>>> On Sat, 17 Feb 2024 19:02:30 +0100
>>> Morten Brørup <mb@smartsharesystems.com> wrote:
>>>
>>>> Not formally... it follows the official DPDK Coding Style [1].
>>>>
>>>> [1]:
>>> https://doc.dpdk.org/guides/contributing/coding_style.html#general
>>>>
>>>>> Should be:
>>>>>
>>>>> if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
>>>>> (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
>>>>> goto clean_txd;
>>>>
>>>> This indentation style is mentioned as an alternative in the guide.
>>> But the example in the guide also uses two tabs for a similar long
>>> comparison.
>>>>
>>>> Personally, I also prefer the style suggested by Stephen, so we might
>>> want to update the Coding Style. ;-)
>>>
>>>
>>> The two tabs is an Intel thing, and I prefer the kernel, line up the
>>> conditional style.
>>
>> I prefer 4 space indentation, which is sufficient to notice the indentation. 8 spaces seems overkill to me, and quickly makes the lines too long.
>> With the editor configured to show tab as 4 spaces, the kernel alignment style ends up with the same indentation for the condition and the code block:
>>
>> if (a &&
>> b)
>> ctr++;
>>
>> Whereas with the "tab as 4 spaces" editor configuration, the double indentation style clearly separates the continued condition from code block:
>>
>> if (a &&
>> b)
>> ctr++;
>>
>
> These two above are the main reasons I much prefer the double indent on
> continuation, though I'd also add a third one: it means we don't have a mix
> of tabs and spaces for indentation.
>
> However, as stated already indent can be a matter of taste, and there will
> be some disagreement about it. The existing coding standards document what
> was done in the code base when they were written, and I don't think we
> should look to change them. It's a bit annoying having 2 standards for
> continuation rather than 1, but it's not exactly a free-for-all, and it's
> not something that applies to every line, only to a small subset.
>
++1
On 2/17/2024 4:47 PM, Stephen Hemminger wrote:
> On Sat, 17 Feb 2024 09:54:10 +0800
> Chaoyong He <chaoyong.he@corigine.com> wrote:
>
>> Add the support of UDP fragmentation offload feature.
>>
>> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
>> Reviewed-by: Long Wu <long.wu@corigine.com>
>> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>> ---
>> drivers/common/nfp/nfp_common_ctrl.h | 1 +
>> drivers/net/nfp/nfd3/nfp_nfd3_dp.c | 7 ++++++-
>> drivers/net/nfp/nfdk/nfp_nfdk_dp.c | 8 +++++---
>> drivers/net/nfp/nfp_net_common.c | 8 ++++++--
>> 4 files changed, 18 insertions(+), 6 deletions(-)
> Looks like this depends on earlier patch, it does not apply directly to main branch.
>
It is a drivers/net patch, so should be on top of next-net, and applies
clean to next-net.
CI also detects sub-tree correctly and able to apply and test the patch.
On 2/17/2024 1:54 AM, Chaoyong He wrote:
> Add the support of UDP fragmentation offload feature.
>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
<...>
> diff --git a/drivers/net/nfp/nfp_net_common.c b/drivers/net/nfp/nfp_net_common.c
> index 72c9a41b00..99c319eb2d 100644
> --- a/drivers/net/nfp/nfp_net_common.c
> +++ b/drivers/net/nfp/nfp_net_common.c
> @@ -312,7 +312,7 @@ nfp_net_log_device_information(const struct nfp_net_hw *hw)
> hw->ver.major, hw->ver.minor, hw->max_mtu);
>
> PMD_INIT_LOG(INFO, "CAP: %#x", cap);
> - PMD_INIT_LOG(INFO, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
> + PMD_INIT_LOG(INFO, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
>
This seems getting out of hand, I assume this is done like this (instead
of multiple print lines) to prevent line break, if so what do you think
add a new macro that doesn't append \n automatically and refactor this
code (in a separate patch) ?
On 2/17/2024 1:54 AM, Chaoyong He wrote:
> Add the support of UDP fragmentation offload feature.
>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>
Applied to dpdk-next-net/main, thanks.
> On 2/17/2024 1:54 AM, Chaoyong He wrote:
> > Add the support of UDP fragmentation offload feature.
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Long Wu <long.wu@corigine.com>
> > Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
>
> <...>
>
> > diff --git a/drivers/net/nfp/nfp_net_common.c
> > b/drivers/net/nfp/nfp_net_common.c
> > index 72c9a41b00..99c319eb2d 100644
> > --- a/drivers/net/nfp/nfp_net_common.c
> > +++ b/drivers/net/nfp/nfp_net_common.c
> > @@ -312,7 +312,7 @@ nfp_net_log_device_information(const struct
> nfp_net_hw *hw)
> > hw->ver.major, hw->ver.minor, hw->max_mtu);
> >
> > PMD_INIT_LOG(INFO, "CAP: %#x", cap);
> > - PMD_INIT_LOG(INFO,
> "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
> > + PMD_INIT_LOG(INFO,
> >
> +"%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s
> %s",
> >
>
> This seems getting out of hand, I assume this is done like this (instead of multiple
> print lines) to prevent line break, if so what do you think add a new macro that
> doesn't append \n automatically and refactor this code (in a separate patch) ?
Yeah, that's a good point, thanks!
I will add it into my to-do list, and send a patch at the suitable time.
@@ -226,6 +226,7 @@ struct nfp_net_fw_ver {
#define NFP_NET_CFG_CTRL_MULTI_PF (0x1 << 5)
#define NFP_NET_CFG_CTRL_FLOW_STEER (0x1 << 8) /**< Flow Steering */
#define NFP_NET_CFG_CTRL_IN_ORDER (0x1 << 11) /**< Virtio in-order flag */
+#define NFP_NET_CFG_CTRL_USO (0x1 << 16) /**< UDP segmentation offload */
#define NFP_NET_CFG_CAP_WORD1 0x00a4
@@ -34,7 +34,8 @@ nfp_net_nfd3_tx_tso(struct nfp_net_txq *txq,
goto clean_txd;
ol_flags = mb->ol_flags;
- if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0)
+ if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
+ (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
goto clean_txd;
txd->l3_offset = mb->l2_len;
@@ -78,6 +79,10 @@ nfp_net_nfd3_tx_cksum(struct nfp_net_txq *txq,
if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) != 0)
txd->flags |= NFD3_DESC_TX_TCP_CSUM;
+ /* Set UDP csum offload if UFO enabled. */
+ if ((ol_flags & RTE_MBUF_F_TX_UDP_SEG) != 0)
+ txd->flags |= NFD3_DESC_TX_UDP_CSUM;
+
/* IPv6 does not need checksum */
if ((ol_flags & RTE_MBUF_F_TX_IP_CKSUM) != 0)
txd->flags |= NFD3_DESC_TX_IP4_CSUM;
@@ -28,8 +28,9 @@ nfp_net_nfdk_tx_cksum(struct nfp_net_txq *txq,
ol_flags = mb->ol_flags;
- /* Set TCP csum offload if TSO enabled. */
- if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) != 0)
+ /* Set L4 csum offload if TSO/UFO enabled. */
+ if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) != 0 ||
+ (ol_flags & RTE_MBUF_F_TX_UDP_SEG) != 0)
flags |= NFDK_DESC_TX_L4_CSUM;
if ((ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK) != 0)
@@ -61,7 +62,8 @@ nfp_net_nfdk_tx_tso(struct nfp_net_txq *txq,
return txd.raw;
ol_flags = mb->ol_flags;
- if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0)
+ if ((ol_flags & RTE_MBUF_F_TX_TCP_SEG) == 0 &&
+ (ol_flags & RTE_MBUF_F_TX_UDP_SEG) == 0)
return txd.raw;
txd.l3_offset = mb->l2_len;
@@ -312,7 +312,7 @@ nfp_net_log_device_information(const struct nfp_net_hw *hw)
hw->ver.major, hw->ver.minor, hw->max_mtu);
PMD_INIT_LOG(INFO, "CAP: %#x", cap);
- PMD_INIT_LOG(INFO, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
+ PMD_INIT_LOG(INFO, "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
cap & NFP_NET_CFG_CTRL_ENABLE ? "ENABLE " : "",
cap & NFP_NET_CFG_CTRL_PROMISC ? "PROMISC " : "",
cap & NFP_NET_CFG_CTRL_L2BC ? "L2BCFILT " : "",
@@ -340,7 +340,8 @@ nfp_net_log_device_information(const struct nfp_net_hw *hw)
cap & NFP_NET_CFG_CTRL_LSO2 ? "TSOv2 " : "",
cap & NFP_NET_CFG_CTRL_RSS2 ? "RSSv2 " : "",
cap & NFP_NET_CFG_CTRL_CSUM_COMPLETE ? "CSUM " : "",
- cap & NFP_NET_CFG_CTRL_LIVE_ADDR ? "LIVE_ADDR " : "");
+ cap & NFP_NET_CFG_CTRL_LIVE_ADDR ? "LIVE_ADDR " : "",
+ cap & NFP_NET_CFG_CTRL_USO ? "USO" : "");
PMD_INIT_LOG(INFO, "CAP_WORD1: %#x", cap_ext);
PMD_INIT_LOG(INFO, "%s%s%s%s%s%s%s",
@@ -537,6 +538,7 @@ nfp_check_offloads(struct rte_eth_dev *dev)
/* LSO offload */
if ((tx_offload & RTE_ETH_TX_OFFLOAD_TCP_TSO) != 0 ||
+ (tx_offload & RTE_ETH_TX_OFFLOAD_UDP_TSO) != 0 ||
(tx_offload & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO) != 0) {
if ((cap & NFP_NET_CFG_CTRL_LSO) != 0)
ctrl |= NFP_NET_CFG_CTRL_LSO;
@@ -1214,6 +1216,8 @@ nfp_net_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
if ((cap & NFP_NET_CFG_CTRL_LSO_ANY) != 0) {
dev_info->tx_offload_capa |= RTE_ETH_TX_OFFLOAD_TCP_TSO;
+ if ((cap & NFP_NET_CFG_CTRL_USO) != 0)
+ dev_info->tx_offload_capa |= RTE_ETH_TX_OFFLOAD_UDP_TSO;
if ((cap & NFP_NET_CFG_CTRL_VXLAN) != 0)
dev_info->tx_offload_capa |= RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO;
}