| Message ID | 20230606113533.6866-3-ivan.malov@arknetworks.am |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | DPDK: align flow offloads with 22.11 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
Bleep bloop. Greetings Ivan Malov, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Inappropriate bracing around statement
#28 FILE: lib/netdev-offload-dpdk.c:743:
if (ethdev)
Lines checked: 80, Warnings: 0, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 6/6/23 13:35, Ivan Malov wrote: > Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. AFAICT, not all drivers moved to a REPRESENTED_PORT action. I don't see support in NFP driver, for example. Are we OK with dropping hardware offload support for NFP ? Or are there plans to support this new action in that driver? Simon, maybe you have some visibility on that? Best regards, Ilya Maximets. > > Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> > --- > lib/netdev-offload-dpdk.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c > index 14bc87771..ef0a266c5 100644 > --- a/lib/netdev-offload-dpdk.c > +++ b/lib/netdev-offload-dpdk.c > @@ -735,14 +735,14 @@ dump_flow_action(struct ds *s, struct ds *s_extra, > ds_put_cstr(s, "rss / "); > } else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) { > ds_put_cstr(s, "count / "); > - } else if (actions->type == RTE_FLOW_ACTION_TYPE_PORT_ID) { > - const struct rte_flow_action_port_id *port_id = actions->conf; > + } else if (actions->type == RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT) { > + const struct rte_flow_action_ethdev *ethdev = actions->conf; > + > + ds_put_cstr(s, "represented_port "); > + > + if (ethdev) > + ds_put_format(s, "ethdev_port_id %d ", ethdev->port_id); > > - ds_put_cstr(s, "port_id "); > - if (port_id) { > - ds_put_format(s, "original %d id %d ", > - port_id->original, port_id->id); > - } > ds_put_cstr(s, "/ "); > } else if (actions->type == RTE_FLOW_ACTION_TYPE_DROP) { > ds_put_cstr(s, "drop / "); > @@ -1776,19 +1776,22 @@ add_count_action(struct flow_actions *actions) > } > > static int > -add_port_id_action(struct flow_actions *actions, > - struct netdev *outdev) > +add_represented_port_action(struct flow_actions *actions, > + struct netdev *outdev) > { > - struct rte_flow_action_port_id *port_id; > + struct rte_flow_action_ethdev *ethdev; > int outdev_id; > > outdev_id = netdev_dpdk_get_port_id(outdev); > if (outdev_id < 0) { > return -1; > } > - port_id = xzalloc(sizeof *port_id); > - port_id->id = outdev_id; > - add_flow_action(actions, RTE_FLOW_ACTION_TYPE_PORT_ID, port_id); > + > + ethdev = xzalloc(sizeof *ethdev); > + ethdev->port_id = outdev_id; > + > + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT, ethdev); > + > return 0; > } > > @@ -1808,7 +1811,7 @@ add_output_action(struct netdev *netdev, > return -1; > } > if (!netdev_flow_api_equals(netdev, outdev) || > - add_port_id_action(actions, outdev)) { > + add_represented_port_action(actions, outdev)) { > VLOG_DBG_RL(&rl, "%s: Output to port \'%s\' cannot be offloaded.", > netdev_get_name(netdev), netdev_get_name(outdev)); > ret = -1;
On Thu, Jun 29, 2023 at 04:15:09PM +0200, Ilya Maximets wrote: > On 6/6/23 13:35, Ivan Malov wrote: > > Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. > > AFAICT, not all drivers moved to a REPRESENTED_PORT action. > I don't see support in NFP driver, for example. > > Are we OK with dropping hardware offload support for NFP ? With my Corigine hat on: I would think the answer is no. > Or are there plans to support this new action in that driver? > > Simon, maybe you have some visibility on that? Let me check with the team.
Hi Ilya, Thanks for reviewing this. Please see below. On Thu, 29 Jun 2023, Ilya Maximets wrote: > On 6/6/23 13:35, Ivan Malov wrote: >> Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. > > AFAICT, not all drivers moved to a REPRESENTED_PORT action. > I don't see support in NFP driver, for example. This is the full story: 1) On Oct 13, 2021, commit [1] introduced a deprecation note for action PORT_ID. 2) On Aug 13, 2022, series [2] was sent to provide replacement for action PORT_ID in drivers that still had been using it. The series was accepted on Aug 22. NFP was not among the drivers to address because it had not been using PORT_ID. 3) On Oct 21, 2022, commit [3] added support for action PORT_ID to NFP. Based on this input, it appears that deprecation notice for action PORT_ID might have been ignored by NFP developers for some reason. They added support for this action after series [2] had been applied, so developers behind [2] could not have a chance to offer help. [1] 5da44faa809a ("ethdev: deprecate hard-to-use or ambiguous items and actions") [2] https://patchwork.dpdk.org/project/dpdk/list/?series=24302&archive=both&state=* [3] 4d946034bf9c ("net/nfp: support basic flow actions") > > Are we OK with dropping hardware offload support for NFP ? I'm not saying above explanation justifies dropping such support, but perhaps it pays to ask NFP team for their opinion on this. Thank you. > Or are there plans to support this new action in that driver? > > Simon, maybe you have some visibility on that? > > Best regards, Ilya Maximets. > >> >> Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> >> --- >> lib/netdev-offload-dpdk.c | 31 +++++++++++++++++-------------- >> 1 file changed, 17 insertions(+), 14 deletions(-) >> >> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >> index 14bc87771..ef0a266c5 100644 >> --- a/lib/netdev-offload-dpdk.c >> +++ b/lib/netdev-offload-dpdk.c >> @@ -735,14 +735,14 @@ dump_flow_action(struct ds *s, struct ds *s_extra, >> ds_put_cstr(s, "rss / "); >> } else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) { >> ds_put_cstr(s, "count / "); >> - } else if (actions->type == RTE_FLOW_ACTION_TYPE_PORT_ID) { >> - const struct rte_flow_action_port_id *port_id = actions->conf; >> + } else if (actions->type == RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT) { >> + const struct rte_flow_action_ethdev *ethdev = actions->conf; >> + >> + ds_put_cstr(s, "represented_port "); >> + >> + if (ethdev) >> + ds_put_format(s, "ethdev_port_id %d ", ethdev->port_id); >> >> - ds_put_cstr(s, "port_id "); >> - if (port_id) { >> - ds_put_format(s, "original %d id %d ", >> - port_id->original, port_id->id); >> - } >> ds_put_cstr(s, "/ "); >> } else if (actions->type == RTE_FLOW_ACTION_TYPE_DROP) { >> ds_put_cstr(s, "drop / "); >> @@ -1776,19 +1776,22 @@ add_count_action(struct flow_actions *actions) >> } >> >> static int >> -add_port_id_action(struct flow_actions *actions, >> - struct netdev *outdev) >> +add_represented_port_action(struct flow_actions *actions, >> + struct netdev *outdev) >> { >> - struct rte_flow_action_port_id *port_id; >> + struct rte_flow_action_ethdev *ethdev; >> int outdev_id; >> >> outdev_id = netdev_dpdk_get_port_id(outdev); >> if (outdev_id < 0) { >> return -1; >> } >> - port_id = xzalloc(sizeof *port_id); >> - port_id->id = outdev_id; >> - add_flow_action(actions, RTE_FLOW_ACTION_TYPE_PORT_ID, port_id); >> + >> + ethdev = xzalloc(sizeof *ethdev); >> + ethdev->port_id = outdev_id; >> + >> + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT, ethdev); >> + >> return 0; >> } >> >> @@ -1808,7 +1811,7 @@ add_output_action(struct netdev *netdev, >> return -1; >> } >> if (!netdev_flow_api_equals(netdev, outdev) || >> - add_port_id_action(actions, outdev)) { >> + add_represented_port_action(actions, outdev)) { >> VLOG_DBG_RL(&rl, "%s: Output to port \'%s\' cannot be offloaded.", >> netdev_get_name(netdev), netdev_get_name(outdev)); >> ret = -1; > >
On Thu, Jun 29, 2023 at 04:55:25PM +0200, Simon Horman wrote: > On Thu, Jun 29, 2023 at 04:15:09PM +0200, Ilya Maximets wrote: > > On 6/6/23 13:35, Ivan Malov wrote: > > > Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. > > > > AFAICT, not all drivers moved to a REPRESENTED_PORT action. > > I don't see support in NFP driver, for example. > > > > Are we OK with dropping hardware offload support for NFP ? > > With my Corigine hat on: > > I would think the answer is no. > > > Or are there plans to support this new action in that driver? > > > > Simon, maybe you have some visibility on that? > > Let me check with the team. Hi Ilya, Ivan, all, I checked with the team at Corigine. They don't see any problem with updating the NFP PMD to use REPRESENTED_PORT instead of PORT_ID. And I think we can assume that work will happen in a timely manner. But there is a question about how to make the transition. My understanding is that from a Corigine perspective, it would be best if OvS could add support for REPRESENTED_PORT and then later remove support for PORT_ID.
On 7/3/23 10:45, Simon Horman wrote: > On Thu, Jun 29, 2023 at 04:55:25PM +0200, Simon Horman wrote: >> On Thu, Jun 29, 2023 at 04:15:09PM +0200, Ilya Maximets wrote: >>> On 6/6/23 13:35, Ivan Malov wrote: >>>> Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. >>> >>> AFAICT, not all drivers moved to a REPRESENTED_PORT action. >>> I don't see support in NFP driver, for example. >>> >>> Are we OK with dropping hardware offload support for NFP ? >> >> With my Corigine hat on: >> >> I would think the answer is no. >> >>> Or are there plans to support this new action in that driver? >>> >>> Simon, maybe you have some visibility on that? >> >> Let me check with the team. > > Hi Ilya, Ivan, all, > > I checked with the team at Corigine. > > They don't see any problem with updating the NFP PMD to use > REPRESENTED_PORT instead of PORT_ID. And I think we can assume that work > will happen in a timely manner. But there is a question about how to make > the transition. > > My understanding is that from a Corigine perspective, it would be best if > OvS could add support for REPRESENTED_PORT and then later remove support > for PORT_ID. We could, in theory, probe the driver by trying to create dummy rte_flow rules. But that sounds like a lot of unnecessary work. If Corigine team can commit to update the driver before 23.11 release, we can switch OVS to REPRESENTED_PORT while moving to 23.11. All the current DPDK drivers that support REPRESENTED_PORT do support PORT_ID as well, AFAICT, except for ICE driver. But nobody complained about non-working offload with ICE, so there is no point trading it for NFP right now. If we make the change while moving to 23.11 we'll hopefully not need to drop support for any driver. And we'll gain support for ICE then. Thoughts? Best regards, Ilya Maximets.
On Mon, Jul 03, 2023 at 01:28:06PM +0200, Ilya Maximets wrote: > On 7/3/23 10:45, Simon Horman wrote: > > On Thu, Jun 29, 2023 at 04:55:25PM +0200, Simon Horman wrote: > >> On Thu, Jun 29, 2023 at 04:15:09PM +0200, Ilya Maximets wrote: > >>> On 6/6/23 13:35, Ivan Malov wrote: > >>>> Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. > >>> > >>> AFAICT, not all drivers moved to a REPRESENTED_PORT action. > >>> I don't see support in NFP driver, for example. > >>> > >>> Are we OK with dropping hardware offload support for NFP ? > >> > >> With my Corigine hat on: > >> > >> I would think the answer is no. > >> > >>> Or are there plans to support this new action in that driver? > >>> > >>> Simon, maybe you have some visibility on that? > >> > >> Let me check with the team. > > > > Hi Ilya, Ivan, all, > > > > I checked with the team at Corigine. > > > > They don't see any problem with updating the NFP PMD to use > > REPRESENTED_PORT instead of PORT_ID. And I think we can assume that work > > will happen in a timely manner. But there is a question about how to make > > the transition. > > > > My understanding is that from a Corigine perspective, it would be best if > > OvS could add support for REPRESENTED_PORT and then later remove support > > for PORT_ID. > > We could, in theory, probe the driver by trying to create dummy rte_flow > rules. But that sounds like a lot of unnecessary work. > > If Corigine team can commit to update the driver before 23.11 release, > we can switch OVS to REPRESENTED_PORT while moving to 23.11. > > All the current DPDK drivers that support REPRESENTED_PORT do support > PORT_ID as well, AFAICT, except for ICE driver. But nobody complained > about non-working offload with ICE, so there is no point trading it > for NFP right now. > > If we make the change while moving to 23.11 we'll hopefully not need to > drop support for any driver. And we'll gain support for ICE then. > > Thoughts? Thanks Ilya, Sounds good. I believe Corigine can work with that plan.
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c index 14bc87771..ef0a266c5 100644 --- a/lib/netdev-offload-dpdk.c +++ b/lib/netdev-offload-dpdk.c @@ -735,14 +735,14 @@ dump_flow_action(struct ds *s, struct ds *s_extra, ds_put_cstr(s, "rss / "); } else if (actions->type == RTE_FLOW_ACTION_TYPE_COUNT) { ds_put_cstr(s, "count / "); - } else if (actions->type == RTE_FLOW_ACTION_TYPE_PORT_ID) { - const struct rte_flow_action_port_id *port_id = actions->conf; + } else if (actions->type == RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT) { + const struct rte_flow_action_ethdev *ethdev = actions->conf; + + ds_put_cstr(s, "represented_port "); + + if (ethdev) + ds_put_format(s, "ethdev_port_id %d ", ethdev->port_id); - ds_put_cstr(s, "port_id "); - if (port_id) { - ds_put_format(s, "original %d id %d ", - port_id->original, port_id->id); - } ds_put_cstr(s, "/ "); } else if (actions->type == RTE_FLOW_ACTION_TYPE_DROP) { ds_put_cstr(s, "drop / "); @@ -1776,19 +1776,22 @@ add_count_action(struct flow_actions *actions) } static int -add_port_id_action(struct flow_actions *actions, - struct netdev *outdev) +add_represented_port_action(struct flow_actions *actions, + struct netdev *outdev) { - struct rte_flow_action_port_id *port_id; + struct rte_flow_action_ethdev *ethdev; int outdev_id; outdev_id = netdev_dpdk_get_port_id(outdev); if (outdev_id < 0) { return -1; } - port_id = xzalloc(sizeof *port_id); - port_id->id = outdev_id; - add_flow_action(actions, RTE_FLOW_ACTION_TYPE_PORT_ID, port_id); + + ethdev = xzalloc(sizeof *ethdev); + ethdev->port_id = outdev_id; + + add_flow_action(actions, RTE_FLOW_ACTION_TYPE_REPRESENTED_PORT, ethdev); + return 0; } @@ -1808,7 +1811,7 @@ add_output_action(struct netdev *netdev, return -1; } if (!netdev_flow_api_equals(netdev, outdev) || - add_port_id_action(actions, outdev)) { + add_represented_port_action(actions, outdev)) { VLOG_DBG_RL(&rl, "%s: Output to port \'%s\' cannot be offloaded.", netdev_get_name(netdev), netdev_get_name(outdev)); ret = -1;
Action PORT_ID has been deprecated. Use REPRESENTED_PORT instead. Signed-off-by: Ivan Malov <ivan.malov@arknetworks.am> --- lib/netdev-offload-dpdk.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)