| Message ID | 20181018132921.31875-2-i.maximets@samsung.com |
|---|---|
| State | Accepted |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | netdev-dpdk: Offload API fixes. | expand |
> vhost ports are not DPDK eth ports and has no rte_flow API. > Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time wasting > and errors in log. > > Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, because > there is no need to expose it in header. Adding Ophir and Tiago as they are both looking at the HWOL work at the moment. > > CC: Finn Christensen <fc@napatech.com> > Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- > lib/netdev-dpdk.c | 10 +++++++--- > lib/netdev-dpdk.h | 4 ---- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > f91aa27cd..7fe5eb087 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev *netdev, const > ovs_u128 *ufid, > ufid, rte_flow); } > > +#define DPDK_FLOW_OFFLOAD_API \ > + .flow_put = netdev_dpdk_flow_put, \ > + .flow_del = netdev_dpdk_flow_del > + > #define NETDEV_DPDK_CLASS_COMMON \ > .is_pmd = true, \ > .alloc = netdev_dpdk_alloc, \ > @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev *netdev, const > ovs_u128 *ufid, > .rxq_alloc = netdev_dpdk_rxq_alloc, \ > .rxq_construct = netdev_dpdk_rxq_construct, \ > .rxq_destruct = netdev_dpdk_rxq_destruct, \ > - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ > - DPDK_FLOW_OFFLOAD_API > + .rxq_dealloc = netdev_dpdk_rxq_dealloc > > #define NETDEV_DPDK_CLASS_BASE \ > NETDEV_DPDK_CLASS_COMMON, \ > @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev *netdev, const > ovs_u128 *ufid, > .get_features = netdev_dpdk_get_features, \ > .get_status = netdev_dpdk_get_status, \ > .reconfigure = netdev_dpdk_reconfigure, \ > - .rxq_recv = netdev_dpdk_rxq_recv > + .rxq_recv = netdev_dpdk_rxq_recv, \ > + DPDK_FLOW_OFFLOAD_API > > static const struct netdev_class dpdk_class = { > .type = "dpdk", > diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index > cc0501d68..b7d02a77d 100644 > --- a/lib/netdev-dpdk.h > +++ b/lib/netdev-dpdk.h > @@ -25,10 +25,6 @@ struct dp_packet; > > #ifdef DPDK_NETDEV > > -#define DPDK_FLOW_OFFLOAD_API \ > - .flow_put = netdev_dpdk_flow_put, \ > - .flow_del = netdev_dpdk_flow_del > - I believe the original reason for placing them here was to keep the approach uniform across the netdevs WRT where the APIs are defined. I believe the expectation is that they will be shared outside of netdev-dpdk at some point. That work in ongoing. https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350510.html We can remove it from mast but FYI it will be back I suspect. Ian > void netdev_dpdk_register(void); > void free_dpdk_buf(struct dp_packet *); > > -- > 2.17.1
On 31.10.2018 13:42, Stokes, Ian wrote: >> vhost ports are not DPDK eth ports and has no rte_flow API. >> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time wasting >> and errors in log. >> >> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, because >> there is no need to expose it in header. > > Adding Ophir and Tiago as they are both looking at the HWOL work at the moment. > >> >> CC: Finn Christensen <fc@napatech.com> >> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >> --- >> lib/netdev-dpdk.c | 10 +++++++--- >> lib/netdev-dpdk.h | 4 ---- >> 2 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >> f91aa27cd..7fe5eb087 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev *netdev, const >> ovs_u128 *ufid, >> ufid, rte_flow); } >> >> +#define DPDK_FLOW_OFFLOAD_API \ >> + .flow_put = netdev_dpdk_flow_put, \ >> + .flow_del = netdev_dpdk_flow_del >> + >> #define NETDEV_DPDK_CLASS_COMMON \ >> .is_pmd = true, \ >> .alloc = netdev_dpdk_alloc, \ >> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev *netdev, const >> ovs_u128 *ufid, >> .rxq_alloc = netdev_dpdk_rxq_alloc, \ >> .rxq_construct = netdev_dpdk_rxq_construct, \ >> .rxq_destruct = netdev_dpdk_rxq_destruct, \ >> - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ >> - DPDK_FLOW_OFFLOAD_API >> + .rxq_dealloc = netdev_dpdk_rxq_dealloc >> >> #define NETDEV_DPDK_CLASS_BASE \ >> NETDEV_DPDK_CLASS_COMMON, \ >> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev *netdev, const >> ovs_u128 *ufid, >> .get_features = netdev_dpdk_get_features, \ >> .get_status = netdev_dpdk_get_status, \ >> .reconfigure = netdev_dpdk_reconfigure, \ >> - .rxq_recv = netdev_dpdk_rxq_recv >> + .rxq_recv = netdev_dpdk_rxq_recv, \ >> + DPDK_FLOW_OFFLOAD_API >> >> static const struct netdev_class dpdk_class = { >> .type = "dpdk", >> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index >> cc0501d68..b7d02a77d 100644 >> --- a/lib/netdev-dpdk.h >> +++ b/lib/netdev-dpdk.h >> @@ -25,10 +25,6 @@ struct dp_packet; >> >> #ifdef DPDK_NETDEV >> >> -#define DPDK_FLOW_OFFLOAD_API \ >> - .flow_put = netdev_dpdk_flow_put, \ >> - .flow_del = netdev_dpdk_flow_del >> - > > I believe the original reason for placing them here was to keep the approach uniform across the netdevs WRT where the APIs are defined. > > I believe the expectation is that they will be shared outside of netdev-dpdk at some point. That work in ongoing. > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350510.html > > We can remove it from mast but FYI it will be back I suspect. The functions are static to netdev-dpdk.c. If you're going to create new DPDK netdev in a separate file, you will have to move much more code and expose many functions in a header file. netdev-linux has the definition in header, because functions are exposed in header too. Here we have just strange define which has no sense for any file that includes netdev-dpdk.h and could not be used anyway without exposing these static functions in header. IMHO, with this change applied, future full offloading patches will be more consistent. > > Ian > >> void netdev_dpdk_register(void); >> void free_dpdk_buf(struct dp_packet *); >> >> -- >> 2.17.1 > > >
> On 31.10.2018 13:42, Stokes, Ian wrote: > >> vhost ports are not DPDK eth ports and has no rte_flow API. > >> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time > >> wasting and errors in log. > >> > >> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > >> because there is no need to expose it in header. > > > > Adding Ophir and Tiago as they are both looking at the HWOL work at the > moment. > > > >> > >> CC: Finn Christensen <fc@napatech.com> > >> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte > >> flow") > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >> --- > >> lib/netdev-dpdk.c | 10 +++++++--- > >> lib/netdev-dpdk.h | 4 ---- > >> 2 files changed, 7 insertions(+), 7 deletions(-) > >> > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > >> f91aa27cd..7fe5eb087 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev *netdev, > >> const > >> ovs_u128 *ufid, > >> ufid, rte_flow); } > >> > >> +#define DPDK_FLOW_OFFLOAD_API \ > >> + .flow_put = netdev_dpdk_flow_put, \ > >> + .flow_del = netdev_dpdk_flow_del > >> + > >> #define NETDEV_DPDK_CLASS_COMMON \ > >> .is_pmd = true, \ > >> .alloc = netdev_dpdk_alloc, \ > >> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev *netdev, > >> const > >> ovs_u128 *ufid, > >> .rxq_alloc = netdev_dpdk_rxq_alloc, \ > >> .rxq_construct = netdev_dpdk_rxq_construct, \ > >> .rxq_destruct = netdev_dpdk_rxq_destruct, \ > >> - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ > >> - DPDK_FLOW_OFFLOAD_API > >> + .rxq_dealloc = netdev_dpdk_rxq_dealloc > >> > >> #define NETDEV_DPDK_CLASS_BASE \ > >> NETDEV_DPDK_CLASS_COMMON, \ > >> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev *netdev, > >> const > >> ovs_u128 *ufid, > >> .get_features = netdev_dpdk_get_features, \ > >> .get_status = netdev_dpdk_get_status, \ > >> .reconfigure = netdev_dpdk_reconfigure, \ > >> - .rxq_recv = netdev_dpdk_rxq_recv > >> + .rxq_recv = netdev_dpdk_rxq_recv, \ > >> + DPDK_FLOW_OFFLOAD_API > >> > >> static const struct netdev_class dpdk_class = { > >> .type = "dpdk", > >> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index > >> cc0501d68..b7d02a77d 100644 > >> --- a/lib/netdev-dpdk.h > >> +++ b/lib/netdev-dpdk.h > >> @@ -25,10 +25,6 @@ struct dp_packet; > >> > >> #ifdef DPDK_NETDEV > >> > >> -#define DPDK_FLOW_OFFLOAD_API \ > >> - .flow_put = netdev_dpdk_flow_put, \ > >> - .flow_del = netdev_dpdk_flow_del > >> - > > > > I believe the original reason for placing them here was to keep the > approach uniform across the netdevs WRT where the APIs are defined. > > > > I believe the expectation is that they will be shared outside of netdev- > dpdk at some point. That work in ongoing. > > > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350510.html > > > > We can remove it from mast but FYI it will be back I suspect. > > The functions are static to netdev-dpdk.c. If you're going to create new > DPDK netdev in a separate file, you will have to move much more code and > expose many functions in a header file. > netdev-linux has the definition in header, because functions are exposed > in header too. Here we have just strange define which has no sense for any > file that includes netdev-dpdk.h and could not be used anyway without > exposing these static functions in header. > > IMHO, with this change applied, future full offloading patches will be > more consistent. OK, that makes sense with what we have to date so I won't block the patch. In previous discussions WRT HWOL there was talk that it would be moved to its own HWOL module, but we can cross that bridge when we come to it. Ian > > > > > Ian > > > >> void netdev_dpdk_register(void); > >> void free_dpdk_buf(struct dp_packet *); > >> > >> -- > >> 2.17.1 > > > > > >
On 31/10/2018 11:40, Stokes, Ian wrote: >> On 31.10.2018 13:42, Stokes, Ian wrote: >>>> vhost ports are not DPDK eth ports and has no rte_flow API. >>>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time >>>> wasting and errors in log. >>>> >>>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, >>>> because there is no need to expose it in header. >>> >>> Adding Ophir and Tiago as they are both looking at the HWOL work at the >> moment. >>> >>>> >>>> CC: Finn Christensen <fc@napatech.com> >>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte >>>> flow") >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>> --- >>>> lib/netdev-dpdk.c | 10 +++++++--- >>>> lib/netdev-dpdk.h | 4 ---- >>>> 2 files changed, 7 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>>> f91aa27cd..7fe5eb087 100644 >>>> --- a/lib/netdev-dpdk.c >>>> +++ b/lib/netdev-dpdk.c >>>> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev *netdev, >>>> const >>>> ovs_u128 *ufid, >>>> ufid, rte_flow); } >>>> >>>> +#define DPDK_FLOW_OFFLOAD_API \ >>>> + .flow_put = netdev_dpdk_flow_put, \ >>>> + .flow_del = netdev_dpdk_flow_del >>>> + >>>> #define NETDEV_DPDK_CLASS_COMMON \ >>>> .is_pmd = true, \ >>>> .alloc = netdev_dpdk_alloc, \ >>>> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev *netdev, >>>> const >>>> ovs_u128 *ufid, >>>> .rxq_alloc = netdev_dpdk_rxq_alloc, \ >>>> .rxq_construct = netdev_dpdk_rxq_construct, \ >>>> .rxq_destruct = netdev_dpdk_rxq_destruct, \ >>>> - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ >>>> - DPDK_FLOW_OFFLOAD_API >>>> + .rxq_dealloc = netdev_dpdk_rxq_dealloc >>>> >>>> #define NETDEV_DPDK_CLASS_BASE \ >>>> NETDEV_DPDK_CLASS_COMMON, \ >>>> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev *netdev, >>>> const >>>> ovs_u128 *ufid, >>>> .get_features = netdev_dpdk_get_features, \ >>>> .get_status = netdev_dpdk_get_status, \ >>>> .reconfigure = netdev_dpdk_reconfigure, \ >>>> - .rxq_recv = netdev_dpdk_rxq_recv >>>> + .rxq_recv = netdev_dpdk_rxq_recv, \ >>>> + DPDK_FLOW_OFFLOAD_API >>>> >>>> static const struct netdev_class dpdk_class = { >>>> .type = "dpdk", >>>> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index >>>> cc0501d68..b7d02a77d 100644 >>>> --- a/lib/netdev-dpdk.h >>>> +++ b/lib/netdev-dpdk.h >>>> @@ -25,10 +25,6 @@ struct dp_packet; >>>> >>>> #ifdef DPDK_NETDEV >>>> >>>> -#define DPDK_FLOW_OFFLOAD_API \ >>>> - .flow_put = netdev_dpdk_flow_put, \ >>>> - .flow_del = netdev_dpdk_flow_del >>>> - >>> >>> I believe the original reason for placing them here was to keep the >> approach uniform across the netdevs WRT where the APIs are defined. >>> >>> I believe the expectation is that they will be shared outside of netdev- >> dpdk at some point. That work in ongoing. >>> >>> https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350510.html >>> >>> We can remove it from mast but FYI it will be back I suspect. >> >> The functions are static to netdev-dpdk.c. If you're going to create new >> DPDK netdev in a separate file, you will have to move much more code and >> expose many functions in a header file. >> netdev-linux has the definition in header, because functions are exposed >> in header too. Here we have just strange define which has no sense for any >> file that includes netdev-dpdk.h and could not be used anyway without >> exposing these static functions in header. >> >> IMHO, with this change applied, future full offloading patches will be >> more consistent. > > OK, that makes sense with what we have to date so I won't block the patch. In previous discussions WRT HWOL there was talk that it would be moved to its own HWOL module, but we can cross that bridge when we come to it. I agree. From what I understand the partial offload implementation will have to be integrated with the full offload solution that has been under development, and which will have its own module. Thus, at that point, it is likely that these definitions can be part of the new module for hardware offload. Thanks, Tiago. > > Ian >> >>> >>> Ian >>> >>>> void netdev_dpdk_register(void); >>>> void free_dpdk_buf(struct dp_packet *); >>>> >>>> -- >>>> 2.17.1 >>> >>> >>>
Hi, It is a good idea to remove the macro from the h file. Please find more comments inline > -----Original Message----- > From: Lam, Tiago [mailto:tiago.lam@intel.com] > Sent: Wednesday, October 31, 2018 2:41 PM > To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets > <i.maximets@samsung.com>; ovs-dev@openvswitch.org; Ophir Munk > <ophirmu@mellanox.com> > Cc: Finn Christensen <fc@napatech.com>; Yuanhan Liu > <yliu@fridaylinux.org>; Shahaf Shuler <shahafs@mellanox.com>; Chandran, > Sugesh <sugesh.chandran@intel.com> > Subject: Re: [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports. > > On 31/10/2018 11:40, Stokes, Ian wrote: > >> On 31.10.2018 13:42, Stokes, Ian wrote: > >>>> vhost ports are not DPDK eth ports and has no rte_flow API. > >>>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time > >>>> wasting and errors in log. > >>>> > >>>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > >>>> because there is no need to expose it in header. > >>> > >>> Adding Ophir and Tiago as they are both looking at the HWOL work at > >>> the > >> moment. > >>> > >>>> > >>>> CC: Finn Christensen <fc@napatech.com> > >>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte > >>>> flow") > >>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >>>> --- > >>>> lib/netdev-dpdk.c | 10 +++++++--- > >>>> lib/netdev-dpdk.h | 4 ---- > >>>> 2 files changed, 7 insertions(+), 7 deletions(-) > >>>> > >>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > >>>> f91aa27cd..7fe5eb087 100644 > >>>> --- a/lib/netdev-dpdk.c > >>>> +++ b/lib/netdev-dpdk.c > >>>> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev > *netdev, > >>>> const > >>>> ovs_u128 *ufid, > >>>> ufid, rte_flow); } > >>>> > >>>> +#define DPDK_FLOW_OFFLOAD_API \ > >>>> + .flow_put = netdev_dpdk_flow_put, \ > >>>> + .flow_del = netdev_dpdk_flow_del > >>>> + Regarding the macro name - I suggest using DPDK_FLOW_API. The offload promise is to scale with the HW abilities. If the HW cannot support the flow there would be no offload (just normal OVS operation). So better omitting "OFFLOAD" from the macro name. > >>>> #define NETDEV_DPDK_CLASS_COMMON \ > >>>> .is_pmd = true, \ > >>>> .alloc = netdev_dpdk_alloc, \ > >>>> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev > *netdev, > >>>> const > >>>> ovs_u128 *ufid, > >>>> .rxq_alloc = netdev_dpdk_rxq_alloc, \ > >>>> .rxq_construct = netdev_dpdk_rxq_construct, \ > >>>> .rxq_destruct = netdev_dpdk_rxq_destruct, \ > >>>> - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ > >>>> - DPDK_FLOW_OFFLOAD_API > >>>> + .rxq_dealloc = netdev_dpdk_rxq_dealloc > >>>> > >>>> #define NETDEV_DPDK_CLASS_BASE \ > >>>> NETDEV_DPDK_CLASS_COMMON, \ > >>>> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev > *netdev, > >>>> const > >>>> ovs_u128 *ufid, > >>>> .get_features = netdev_dpdk_get_features, \ > >>>> .get_status = netdev_dpdk_get_status, \ > >>>> .reconfigure = netdev_dpdk_reconfigure, \ > >>>> - .rxq_recv = netdev_dpdk_rxq_recv > >>>> + .rxq_recv = netdev_dpdk_rxq_recv, \ > >>>> + DPDK_FLOW_OFFLOAD_API > >>>> Here is the only place where the new macro is used and it only has 2 callback assignments. What do you say about directly assigning the 2 callbacks and giving up the macro? Something like: - .rxq_recv = netdev_dpdk_rxq_recv + .rxq_recv = netdev_dpdk_rxq_recv, \ - DPDK_FLOW_OFFLOAD_API + .flow_put = netdev_dpdk_flow_put, \ + .flow_del = netdev_dpdk_flow_del > >>>> static const struct netdev_class dpdk_class = { > >>>> .type = "dpdk", > >>>> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index > >>>> cc0501d68..b7d02a77d 100644 > >>>> --- a/lib/netdev-dpdk.h > >>>> +++ b/lib/netdev-dpdk.h > >>>> @@ -25,10 +25,6 @@ struct dp_packet; > >>>> > >>>> #ifdef DPDK_NETDEV > >>>> > >>>> -#define DPDK_FLOW_OFFLOAD_API \ > >>>> - .flow_put = netdev_dpdk_flow_put, \ > >>>> - .flow_del = netdev_dpdk_flow_del > >>>> - > >>> > >>> I believe the original reason for placing them here was to keep the > >> approach uniform across the netdevs WRT where the APIs are defined. > >>> > >>> I believe the expectation is that they will be shared outside of > >>> netdev- > >> dpdk at some point. That work in ongoing. > >>> > >>> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fm > >>> ail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2018- > August%2F350510.htm > >>> > l&data=02%7C01%7Cophirmu%40mellanox.com%7Cbc9e78988b7b4bf8 > b82408 > >>> > d63f2e28d6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63676 > 5864828 > >>> > 187556&sdata=LiGeqC84qM6VCFUtRgKpLv%2BBrq8if4Kyto8mw2EaNQ > s%3D&am > >>> p;reserved=0 > >>> > >>> We can remove it from mast but FYI it will be back I suspect. > >> > >> The functions are static to netdev-dpdk.c. If you're going to create > >> new DPDK netdev in a separate file, you will have to move much more > >> code and expose many functions in a header file. > >> netdev-linux has the definition in header, because functions are > >> exposed in header too. Here we have just strange define which has no > >> sense for any file that includes netdev-dpdk.h and could not be used > >> anyway without exposing these static functions in header. > >> > >> IMHO, with this change applied, future full offloading patches will > >> be more consistent. > > > > OK, that makes sense with what we have to date so I won't block the > patch. In previous discussions WRT HWOL there was talk that it would be > moved to its own HWOL module, but we can cross that bridge when we > come to it. > > I agree. From what I understand the partial offload implementation will have > to be integrated with the full offload solution that has been under > development, and which will have its own module. Thus, at that point, it is > likely that these definitions can be part of the new module for hardware > offload. > With representors ports (since dpdk 18.08) it seems we can have the current class dpdk for HW offload. The representors ports hide the HW details and make them appear as the same dpdk ports to OVS. However, this is a different discussion ... > Thanks, > Tiago. > > > > > Ian > >> > >>> > >>> Ian > >>> > >>>> void netdev_dpdk_register(void); > >>>> void free_dpdk_buf(struct dp_packet *); > >>>> > >>>> -- > >>>> 2.17.1 > >>> > >>> > >>>
On 31.10.2018 20:01, Ophir Munk wrote: > Hi, > It is a good idea to remove the macro from the h file. > Please find more comments inline > >> -----Original Message----- >> From: Lam, Tiago [mailto:tiago.lam@intel.com] >> Sent: Wednesday, October 31, 2018 2:41 PM >> To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets >> <i.maximets@samsung.com>; ovs-dev@openvswitch.org; Ophir Munk >> <ophirmu@mellanox.com> >> Cc: Finn Christensen <fc@napatech.com>; Yuanhan Liu >> <yliu@fridaylinux.org>; Shahaf Shuler <shahafs@mellanox.com>; Chandran, >> Sugesh <sugesh.chandran@intel.com> >> Subject: Re: [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports. >> >> On 31/10/2018 11:40, Stokes, Ian wrote: >>>> On 31.10.2018 13:42, Stokes, Ian wrote: >>>>>> vhost ports are not DPDK eth ports and has no rte_flow API. >>>>>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time >>>>>> wasting and errors in log. >>>>>> >>>>>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, >>>>>> because there is no need to expose it in header. >>>>> >>>>> Adding Ophir and Tiago as they are both looking at the HWOL work at >>>>> the >>>> moment. >>>>> >>>>>> >>>>>> CC: Finn Christensen <fc@napatech.com> >>>>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte >>>>>> flow") >>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>>>>> --- >>>>>> lib/netdev-dpdk.c | 10 +++++++--- >>>>>> lib/netdev-dpdk.h | 4 ---- >>>>>> 2 files changed, 7 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index >>>>>> f91aa27cd..7fe5eb087 100644 >>>>>> --- a/lib/netdev-dpdk.c >>>>>> +++ b/lib/netdev-dpdk.c >>>>>> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev >> *netdev, >>>>>> const >>>>>> ovs_u128 *ufid, >>>>>> ufid, rte_flow); } >>>>>> >>>>>> +#define DPDK_FLOW_OFFLOAD_API \ >>>>>> + .flow_put = netdev_dpdk_flow_put, \ >>>>>> + .flow_del = netdev_dpdk_flow_del >>>>>> + > > Regarding the macro name - I suggest using DPDK_FLOW_API. > The offload promise is to scale with the HW abilities. > If the HW cannot support the flow there would be no offload (just normal OVS operation). > So better omitting "OFFLOAD" from the macro name. IMHO, it's better not to rename. At first, all the corresponding macro definitions for other netdevs has OFFLOAD in their names. At second, this API is intended for offloading and has no sense without it. Most of the users of this API has 'offload' in function and data structure names. > >>>>>> #define NETDEV_DPDK_CLASS_COMMON \ >>>>>> .is_pmd = true, \ >>>>>> .alloc = netdev_dpdk_alloc, \ >>>>>> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev >> *netdev, >>>>>> const >>>>>> ovs_u128 *ufid, >>>>>> .rxq_alloc = netdev_dpdk_rxq_alloc, \ >>>>>> .rxq_construct = netdev_dpdk_rxq_construct, \ >>>>>> .rxq_destruct = netdev_dpdk_rxq_destruct, \ >>>>>> - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ >>>>>> - DPDK_FLOW_OFFLOAD_API >>>>>> + .rxq_dealloc = netdev_dpdk_rxq_dealloc >>>>>> >>>>>> #define NETDEV_DPDK_CLASS_BASE \ >>>>>> NETDEV_DPDK_CLASS_COMMON, \ >>>>>> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev >> *netdev, >>>>>> const >>>>>> ovs_u128 *ufid, >>>>>> .get_features = netdev_dpdk_get_features, \ >>>>>> .get_status = netdev_dpdk_get_status, \ >>>>>> .reconfigure = netdev_dpdk_reconfigure, \ >>>>>> - .rxq_recv = netdev_dpdk_rxq_recv >>>>>> + .rxq_recv = netdev_dpdk_rxq_recv, \ >>>>>> + DPDK_FLOW_OFFLOAD_API >>>>>> > > Here is the only place where the new macro is used and it only has 2 callback assignments. > What do you say about directly assigning the 2 callbacks and giving up the macro? > Something like: > - .rxq_recv = netdev_dpdk_rxq_recv > + .rxq_recv = netdev_dpdk_rxq_recv, \ > - DPDK_FLOW_OFFLOAD_API > + .flow_put = netdev_dpdk_flow_put, \ > + .flow_del = netdev_dpdk_flow_del Not sure. Having a macro here groups the offload API together and makes it visible for a reader. And, probably, we'll implement more offload APIs in the future and there will be more functions. > >>>>>> static const struct netdev_class dpdk_class = { >>>>>> .type = "dpdk", >>>>>> diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index >>>>>> cc0501d68..b7d02a77d 100644 >>>>>> --- a/lib/netdev-dpdk.h >>>>>> +++ b/lib/netdev-dpdk.h >>>>>> @@ -25,10 +25,6 @@ struct dp_packet; >>>>>> >>>>>> #ifdef DPDK_NETDEV >>>>>> >>>>>> -#define DPDK_FLOW_OFFLOAD_API \ >>>>>> - .flow_put = netdev_dpdk_flow_put, \ >>>>>> - .flow_del = netdev_dpdk_flow_del >>>>>> - >>>>> >>>>> I believe the original reason for placing them here was to keep the >>>> approach uniform across the netdevs WRT where the APIs are defined. >>>>> >>>>> I believe the expectation is that they will be shared outside of >>>>> netdev- >>>> dpdk at some point. That work in ongoing. >>>>> >>>>> >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fm >>>>> ail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2018- >> August%2F350510.htm >>>>> >> l&data=02%7C01%7Cophirmu%40mellanox.com%7Cbc9e78988b7b4bf8 >> b82408 >>>>> >> d63f2e28d6%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C63676 >> 5864828 >>>>> >> 187556&sdata=LiGeqC84qM6VCFUtRgKpLv%2BBrq8if4Kyto8mw2EaNQ >> s%3D&am >>>>> p;reserved=0 >>>>> >>>>> We can remove it from mast but FYI it will be back I suspect. >>>> >>>> The functions are static to netdev-dpdk.c. If you're going to create >>>> new DPDK netdev in a separate file, you will have to move much more >>>> code and expose many functions in a header file. >>>> netdev-linux has the definition in header, because functions are >>>> exposed in header too. Here we have just strange define which has no >>>> sense for any file that includes netdev-dpdk.h and could not be used >>>> anyway without exposing these static functions in header. >>>> >>>> IMHO, with this change applied, future full offloading patches will >>>> be more consistent. >>> >>> OK, that makes sense with what we have to date so I won't block the >> patch. In previous discussions WRT HWOL there was talk that it would be >> moved to its own HWOL module, but we can cross that bridge when we >> come to it. >> >> I agree. From what I understand the partial offload implementation will have >> to be integrated with the full offload solution that has been under >> development, and which will have its own module. Thus, at that point, it is >> likely that these definitions can be part of the new module for hardware >> offload. >> > > With representors ports (since dpdk 18.08) it seems we can have the current class dpdk for HW offload. > The representors ports hide the HW details and make them appear as the same dpdk ports to OVS. > However, this is a different discussion ... > >> Thanks, >> Tiago. >> >>> >>> Ian >>>> >>>>> >>>>> Ian >>>>> >>>>>> void netdev_dpdk_register(void); >>>>>> void free_dpdk_buf(struct dp_packet *); >>>>>> >>>>>> -- >>>>>> 2.17.1 >>>>> >>>>> >>>>>
> On 31.10.2018 20:01, Ophir Munk wrote: > > Hi, > > It is a good idea to remove the macro from the h file. > > Please find more comments inline > > > >> -----Original Message----- > >> From: Lam, Tiago [mailto:tiago.lam@intel.com] > >> Sent: Wednesday, October 31, 2018 2:41 PM > >> To: Stokes, Ian <ian.stokes@intel.com>; Ilya Maximets > >> <i.maximets@samsung.com>; ovs-dev@openvswitch.org; Ophir Munk > >> <ophirmu@mellanox.com> > >> Cc: Finn Christensen <fc@napatech.com>; Yuanhan Liu > >> <yliu@fridaylinux.org>; Shahaf Shuler <shahafs@mellanox.com>; > >> Chandran, Sugesh <sugesh.chandran@intel.com> > >> Subject: Re: [PATCH 1/2] netdev-dpdk: Drop offload API for vhost ports. > >> > >> On 31/10/2018 11:40, Stokes, Ian wrote: > >>>> On 31.10.2018 13:42, Stokes, Ian wrote: > >>>>>> vhost ports are not DPDK eth ports and has no rte_flow API. > >>>>>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time > >>>>>> wasting and errors in log. > >>>>>> > >>>>>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > >>>>>> because there is no need to expose it in header. > >>>>> > >>>>> Adding Ophir and Tiago as they are both looking at the HWOL work > >>>>> at the > >>>> moment. > >>>>> > >>>>>> > >>>>>> CC: Finn Christensen <fc@napatech.com> > >>>>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with > >>>>>> rte > >>>>>> flow") > >>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >>>>>> --- > >>>>>> lib/netdev-dpdk.c | 10 +++++++--- lib/netdev-dpdk.h | 4 ---- > >>>>>> 2 files changed, 7 insertions(+), 7 deletions(-) > >>>>>> > >>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index > >>>>>> f91aa27cd..7fe5eb087 100644 > >>>>>> --- a/lib/netdev-dpdk.c > >>>>>> +++ b/lib/netdev-dpdk.c > >>>>>> @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev > >> *netdev, > >>>>>> const > >>>>>> ovs_u128 *ufid, > >>>>>> ufid, rte_flow); } > >>>>>> > >>>>>> +#define DPDK_FLOW_OFFLOAD_API \ > >>>>>> + .flow_put = netdev_dpdk_flow_put, \ > >>>>>> + .flow_del = netdev_dpdk_flow_del > >>>>>> + > > > > Regarding the macro name - I suggest using DPDK_FLOW_API. > > The offload promise is to scale with the HW abilities. > > If the HW cannot support the flow there would be no offload (just normal > OVS operation). > > So better omitting "OFFLOAD" from the macro name. > > IMHO, it's better not to rename. At first, all the corresponding macro > definitions for other netdevs has OFFLOAD in their names. At second, this > API is intended for offloading and has no sense without it. > Most of the users of this API has 'offload' in function and data structure > names. +1, let's keep them uniform across the netdevs. Ian > > > > >>>>>> #define NETDEV_DPDK_CLASS_COMMON \ > >>>>>> .is_pmd = true, \ > >>>>>> .alloc = netdev_dpdk_alloc, \ > >>>>>> @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev > >> *netdev, > >>>>>> const > >>>>>> ovs_u128 *ufid, > >>>>>> .rxq_alloc = netdev_dpdk_rxq_alloc, \ > >>>>>> .rxq_construct = netdev_dpdk_rxq_construct, \ > >>>>>> .rxq_destruct = netdev_dpdk_rxq_destruct, \ > >>>>>> - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ > >>>>>> - DPDK_FLOW_OFFLOAD_API > >>>>>> + .rxq_dealloc = netdev_dpdk_rxq_dealloc > >>>>>> > >>>>>> #define NETDEV_DPDK_CLASS_BASE \ > >>>>>> NETDEV_DPDK_CLASS_COMMON, \ > >>>>>> @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev > >> *netdev, > >>>>>> const > >>>>>> ovs_u128 *ufid, > >>>>>> .get_features = netdev_dpdk_get_features, \ > >>>>>> .get_status = netdev_dpdk_get_status, \ > >>>>>> .reconfigure = netdev_dpdk_reconfigure, \ > >>>>>> - .rxq_recv = netdev_dpdk_rxq_recv > >>>>>> + .rxq_recv = netdev_dpdk_rxq_recv, \ > >>>>>> + DPDK_FLOW_OFFLOAD_API > >>>>>> > > > > Here is the only place where the new macro is used and it only has 2 > callback assignments. > > What do you say about directly assigning the 2 callbacks and giving up > the macro? > > Something like: > > - .rxq_recv = netdev_dpdk_rxq_recv > > + .rxq_recv = netdev_dpdk_rxq_recv, \ > > - DPDK_FLOW_OFFLOAD_API > > + .flow_put = netdev_dpdk_flow_put, \ > > + .flow_del = netdev_dpdk_flow_del > > Not sure. Having a macro here groups the offload API together and makes it > visible for a reader. And, probably, we'll implement more offload APIs in > the future and there will be more functions. > I'd agree, I believe in the original patch to simplify the netdev classes proposed something similar to using callbacks but ultimately it was decided that the API would be expanded in the future so should be kept. Ian
On 18.10.2018 16:29, Ilya Maximets wrote: > vhost ports are not DPDK eth ports and has no rte_flow API. > Stop calling this API with DPDK_ETH_PORT_ID_INVALID to > avoid time wasting and errors in log. > > Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c > file, because there is no need to expose it in header. > > CC: Finn Christensen <fc@napatech.com> > Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > --- Hi Ian, You didn't backport this patch to 2.10. Do you think that it's not needed or you just missed it while preparing the pull request? Periodic errors in log are a bit annoying. Bets regards, Ilya Maximets.
> On 18.10.2018 16:29, Ilya Maximets wrote: > > vhost ports are not DPDK eth ports and has no rte_flow API. > > Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time > > wasting and errors in log. > > > > Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > > because there is no need to expose it in header. > > > > CC: Finn Christensen <fc@napatech.com> > > Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte > > flow") > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > --- > > Hi Ian, > You didn't backport this patch to 2.10. Do you think that it's not needed > or you just missed it while preparing the pull request? > > Periodic errors in log are a bit annoying. Hi Ilya, The patch above assumes that a previous commit 89c09c1cd1f0 ("netdev: Clean up class initialization.") is in place, however 89c09c1cd1f0 ("netdev: Clean up class initialization.") was never backported to branch 2.10 I had discussed this with Ben but we didn’t see the need. As such the patch does not apply as the netdev dpdk class layout differs. You could submit a specific patch for branch 2.10 with an amended commit message. Alternatively I'm thinking it might make sense to backport 89c09c1cd1f0 as well as the patch above in order to remove the periodic log requests? Ian > > Bets regards, Ilya Maximets.
On 06.11.2018 17:31, Stokes, Ian wrote: >> On 18.10.2018 16:29, Ilya Maximets wrote: >>> vhost ports are not DPDK eth ports and has no rte_flow API. >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time >>> wasting and errors in log. >>> >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, >>> because there is no need to expose it in header. >>> >>> CC: Finn Christensen <fc@napatech.com> >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte >>> flow") >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> >>> --- >> >> Hi Ian, >> You didn't backport this patch to 2.10. Do you think that it's not needed >> or you just missed it while preparing the pull request? >> >> Periodic errors in log are a bit annoying. > > Hi Ilya, > > The patch above assumes that a previous commit 89c09c1cd1f0 ("netdev: Clean up class initialization.") is in place, however 89c09c1cd1f0 ("netdev: Clean up class initialization.") was never backported to branch 2.10 I had discussed this with Ben but we didn’t see the need. > OK. I see. > As such the patch does not apply as the netdev dpdk class layout differs. You could submit a specific patch for branch 2.10 with an amended commit message. > > Alternatively I'm thinking it might make sense to backport 89c09c1cd1f0 as well as the patch above in order to remove the periodic log requests? In this case you'll need to backport also commit 72713c651 ("netdev-bsd: Fix build failure because of undefined NO_OFFLOAD_API."). Maybe we can backport only changes related to netdev-dpdk like this: git cherry-pick -n 89c09c1cd1f0 c0af6425d \ && git reset HEAD \ && git add lib/netdev-dpdk.c \ && git checkout . \ && git commit -sv What do you think? > > Ian > > >> >> Bets regards, Ilya Maximets.
> On 06.11.2018 17:31, Stokes, Ian wrote: > >> On 18.10.2018 16:29, Ilya Maximets wrote: > >>> vhost ports are not DPDK eth ports and has no rte_flow API. > >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time > >>> wasting and errors in log. > >>> > >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > >>> because there is no need to expose it in header. > >>> > >>> CC: Finn Christensen <fc@napatech.com> > >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte > >>> flow") > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > >>> --- > >> > >> Hi Ian, > >> You didn't backport this patch to 2.10. Do you think that it's not > >> needed or you just missed it while preparing the pull request? > >> > >> Periodic errors in log are a bit annoying. > > > > Hi Ilya, > > > > The patch above assumes that a previous commit 89c09c1cd1f0 ("netdev: > Clean up class initialization.") is in place, however 89c09c1cd1f0 > ("netdev: Clean up class initialization.") was never backported to branch > 2.10 I had discussed this with Ben but we didn’t see the need. > > > > OK. I see. > > > As such the patch does not apply as the netdev dpdk class layout > differs. You could submit a specific patch for branch 2.10 with an amended > commit message. > > > > Alternatively I'm thinking it might make sense to backport 89c09c1cd1f0 > as well as the patch above in order to remove the periodic log requests? > > In this case you'll need to backport also commit > 72713c651 ("netdev-bsd: Fix build failure because of undefined > NO_OFFLOAD_API."). > > Maybe we can backport only changes related to netdev-dpdk like this: > git cherry-pick -n 89c09c1cd1f0 c0af6425d \ > && git reset HEAD \ > && git add lib/netdev-dpdk.c \ > && git checkout . \ > && git commit -sv > > What do you think? Sure, that would work, I'd like to ask Bens opinion here also. Ben would above be ok or is it preferred to backport commits separately to 2.10 as 89c09c1cd1f0 ("netdev: Clean up class initialization.") 72713c651 ("netdev-bsd: Fix build failure because of undefined NO_OFFLOAD_API."). c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") Is there a preference as regards keeping the original commits and sign off tags for the code changes when backporting rather than creating a new commit that combines code changes. Thanks Ian
On Thu, Nov 08, 2018 at 02:16:15PM +0000, Stokes, Ian wrote: > > On 06.11.2018 17:31, Stokes, Ian wrote: > > >> On 18.10.2018 16:29, Ilya Maximets wrote: > > >>> vhost ports are not DPDK eth ports and has no rte_flow API. > > >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time > > >>> wasting and errors in log. > > >>> > > >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > > >>> because there is no need to expose it in header. > > >>> > > >>> CC: Finn Christensen <fc@napatech.com> > > >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte > > >>> flow") > > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > >>> --- > > >> > > >> Hi Ian, > > >> You didn't backport this patch to 2.10. Do you think that it's not > > >> needed or you just missed it while preparing the pull request? > > >> > > >> Periodic errors in log are a bit annoying. > > > > > > Hi Ilya, > > > > > > The patch above assumes that a previous commit 89c09c1cd1f0 ("netdev: > > Clean up class initialization.") is in place, however 89c09c1cd1f0 > > ("netdev: Clean up class initialization.") was never backported to branch > > 2.10 I had discussed this with Ben but we didn’t see the need. > > > > > > > OK. I see. > > > > > As such the patch does not apply as the netdev dpdk class layout > > differs. You could submit a specific patch for branch 2.10 with an amended > > commit message. > > > > > > Alternatively I'm thinking it might make sense to backport 89c09c1cd1f0 > > as well as the patch above in order to remove the periodic log requests? > > > > In this case you'll need to backport also commit > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > NO_OFFLOAD_API."). > > > > Maybe we can backport only changes related to netdev-dpdk like this: > > git cherry-pick -n 89c09c1cd1f0 c0af6425d \ > > && git reset HEAD \ > > && git add lib/netdev-dpdk.c \ > > && git checkout . \ > > && git commit -sv > > > > What do you think? > > Sure, that would work, I'd like to ask Bens opinion here also. > > Ben would above be ok or is it preferred to backport commits separately to 2.10 as > > 89c09c1cd1f0 ("netdev: Clean up class initialization.") > 72713c651 ("netdev-bsd: Fix build failure because of undefined NO_OFFLOAD_API."). > c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") > > Is there a preference as regards keeping the original commits and sign off tags for the code changes when backporting rather than creating a new commit that combines code changes. Usually we backport commit individually because it makes it easier to figure out what was backported. Individual commits are easy to see at a glance in the history, squashed commits take a closer look.
> On Thu, Nov 08, 2018 at 02:16:15PM +0000, Stokes, Ian wrote: > > > On 06.11.2018 17:31, Stokes, Ian wrote: > > > >> On 18.10.2018 16:29, Ilya Maximets wrote: > > > >>> vhost ports are not DPDK eth ports and has no rte_flow API. > > > >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid > > > >>> time wasting and errors in log. > > > >>> > > > >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > > > >>> because there is no need to expose it in header. > > > >>> > > > >>> CC: Finn Christensen <fc@napatech.com> > > > >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with > > > >>> rte > > > >>> flow") > > > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > >>> --- > > > >> > > > >> Hi Ian, > > > >> You didn't backport this patch to 2.10. Do you think that it's > > > >> not needed or you just missed it while preparing the pull request? > > > >> > > > >> Periodic errors in log are a bit annoying. > > > > > > > > Hi Ilya, > > > > > > > > The patch above assumes that a previous commit 89c09c1cd1f0 > ("netdev: > > > Clean up class initialization.") is in place, however 89c09c1cd1f0 > > > ("netdev: Clean up class initialization.") was never backported to > > > branch > > > 2.10 I had discussed this with Ben but we didn’t see the need. > > > > > > > > > > OK. I see. > > > > > > > As such the patch does not apply as the netdev dpdk class layout > > > differs. You could submit a specific patch for branch 2.10 with an > > > amended commit message. > > > > > > > > Alternatively I'm thinking it might make sense to backport > > > > 89c09c1cd1f0 > > > as well as the patch above in order to remove the periodic log > requests? > > > > > > In this case you'll need to backport also commit > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > > NO_OFFLOAD_API."). > > > > > > Maybe we can backport only changes related to netdev-dpdk like this: > > > git cherry-pick -n 89c09c1cd1f0 c0af6425d \ > > > && git reset HEAD \ > > > && git add lib/netdev-dpdk.c \ > > > && git checkout . \ > > > && git commit -sv > > > > > > What do you think? > > > > Sure, that would work, I'd like to ask Bens opinion here also. > > > > Ben would above be ok or is it preferred to backport commits > > separately to 2.10 as > > > > 89c09c1cd1f0 ("netdev: Clean up class initialization.") > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > NO_OFFLOAD_API."). > > c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") > > > > Is there a preference as regards keeping the original commits and sign > off tags for the code changes when backporting rather than creating a new > commit that combines code changes. > > Usually we backport commit individually because it makes it easier to > figure out what was backported. Individual commits are easy to see at a > glance in the history, squashed commits take a closer look. Thanks for clarifying, in that case I'll backport the three commits separately. A quick follow up, commit 89c09c1cd1f0 isn't specific to netdev-dpdk and also is not a bug fix but it does help avoid periodic errors in the logs for HWOL. As it doesn't add new functionality I would assume it's ok to backport? Thanks Ian
On Thu, Nov 08, 2018 at 02:40:12PM +0000, Stokes, Ian wrote: > > On Thu, Nov 08, 2018 at 02:16:15PM +0000, Stokes, Ian wrote: > > > > On 06.11.2018 17:31, Stokes, Ian wrote: > > > > >> On 18.10.2018 16:29, Ilya Maximets wrote: > > > > >>> vhost ports are not DPDK eth ports and has no rte_flow API. > > > > >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid > > > > >>> time wasting and errors in log. > > > > >>> > > > > >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, > > > > >>> because there is no need to expose it in header. > > > > >>> > > > > >>> CC: Finn Christensen <fc@napatech.com> > > > > >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with > > > > >>> rte > > > > >>> flow") > > > > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > > >>> --- > > > > >> > > > > >> Hi Ian, > > > > >> You didn't backport this patch to 2.10. Do you think that it's > > > > >> not needed or you just missed it while preparing the pull request? > > > > >> > > > > >> Periodic errors in log are a bit annoying. > > > > > > > > > > Hi Ilya, > > > > > > > > > > The patch above assumes that a previous commit 89c09c1cd1f0 > > ("netdev: > > > > Clean up class initialization.") is in place, however 89c09c1cd1f0 > > > > ("netdev: Clean up class initialization.") was never backported to > > > > branch > > > > 2.10 I had discussed this with Ben but we didn’t see the need. > > > > > > > > > > > > > OK. I see. > > > > > > > > > As such the patch does not apply as the netdev dpdk class layout > > > > differs. You could submit a specific patch for branch 2.10 with an > > > > amended commit message. > > > > > > > > > > Alternatively I'm thinking it might make sense to backport > > > > > 89c09c1cd1f0 > > > > as well as the patch above in order to remove the periodic log > > requests? > > > > > > > > In this case you'll need to backport also commit > > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > > > NO_OFFLOAD_API."). > > > > > > > > Maybe we can backport only changes related to netdev-dpdk like this: > > > > git cherry-pick -n 89c09c1cd1f0 c0af6425d \ > > > > && git reset HEAD \ > > > > && git add lib/netdev-dpdk.c \ > > > > && git checkout . \ > > > > && git commit -sv > > > > > > > > What do you think? > > > > > > Sure, that would work, I'd like to ask Bens opinion here also. > > > > > > Ben would above be ok or is it preferred to backport commits > > > separately to 2.10 as > > > > > > 89c09c1cd1f0 ("netdev: Clean up class initialization.") > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > NO_OFFLOAD_API."). > > > c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") > > > > > > Is there a preference as regards keeping the original commits and sign > > off tags for the code changes when backporting rather than creating a new > > commit that combines code changes. > > > > Usually we backport commit individually because it makes it easier to > > figure out what was backported. Individual commits are easy to see at a > > glance in the history, squashed commits take a closer look. > > Thanks for clarifying, in that case I'll backport the three commits separately. > > A quick follow up, commit 89c09c1cd1f0 isn't specific to netdev-dpdk and also is not a bug fix but it does help avoid periodic errors in the logs for HWOL. As it doesn't add new functionality I would assume it's ok to backport? Seems fine to me.
> On Thu, Nov 08, 2018 at 02:40:12PM +0000, Stokes, Ian wrote: > > > On Thu, Nov 08, 2018 at 02:16:15PM +0000, Stokes, Ian wrote: > > > > > On 06.11.2018 17:31, Stokes, Ian wrote: > > > > > >> On 18.10.2018 16:29, Ilya Maximets wrote: > > > > > >>> vhost ports are not DPDK eth ports and has no rte_flow API. > > > > > >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid > > > > > >>> time wasting and errors in log. > > > > > >>> > > > > > >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c > > > > > >>> file, because there is no need to expose it in header. > > > > > >>> > > > > > >>> CC: Finn Christensen <fc@napatech.com> > > > > > >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload > > > > > >>> with rte > > > > > >>> flow") > > > > > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > > > >>> --- > > > > > >> > > > > > >> Hi Ian, > > > > > >> You didn't backport this patch to 2.10. Do you think that > > > > > >> it's not needed or you just missed it while preparing the pull > request? > > > > > >> > > > > > >> Periodic errors in log are a bit annoying. > > > > > > > > > > > > Hi Ilya, > > > > > > > > > > > > The patch above assumes that a previous commit 89c09c1cd1f0 > > > ("netdev: > > > > > Clean up class initialization.") is in place, however > > > > > 89c09c1cd1f0 > > > > > ("netdev: Clean up class initialization.") was never backported > > > > > to branch > > > > > 2.10 I had discussed this with Ben but we didn’t see the need. > > > > > > > > > > > > > > > > OK. I see. > > > > > > > > > > > As such the patch does not apply as the netdev dpdk class > > > > > > layout > > > > > differs. You could submit a specific patch for branch 2.10 with > > > > > an amended commit message. > > > > > > > > > > > > Alternatively I'm thinking it might make sense to backport > > > > > > 89c09c1cd1f0 > > > > > as well as the patch above in order to remove the periodic log > > > requests? > > > > > > > > > > In this case you'll need to backport also commit > > > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > > > > NO_OFFLOAD_API."). > > > > > > > > > > Maybe we can backport only changes related to netdev-dpdk like > this: > > > > > git cherry-pick -n 89c09c1cd1f0 c0af6425d \ > > > > > && git reset HEAD \ > > > > > && git add lib/netdev-dpdk.c \ > > > > > && git checkout . \ > > > > > && git commit -sv > > > > > > > > > > What do you think? > > > > > > > > Sure, that would work, I'd like to ask Bens opinion here also. > > > > > > > > Ben would above be ok or is it preferred to backport commits > > > > separately to 2.10 as > > > > > > > > 89c09c1cd1f0 ("netdev: Clean up class initialization.") > > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > > NO_OFFLOAD_API."). > > > > c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") > > > > > > > > Is there a preference as regards keeping the original commits and > > > > sign > > > off tags for the code changes when backporting rather than creating > > > a new commit that combines code changes. > > > > > > Usually we backport commit individually because it makes it easier > > > to figure out what was backported. Individual commits are easy to > > > see at a glance in the history, squashed commits take a closer look. > > > > Thanks for clarifying, in that case I'll backport the three commits > separately. > > > > A quick follow up, commit 89c09c1cd1f0 isn't specific to netdev-dpdk and > also is not a bug fix but it does help avoid periodic errors in the logs > for HWOL. As it doesn't add new functionality I would assume it's ok to > backport? > > Seems fine to me. Thanks Ben, Ian
> On Thu, Nov 08, 2018 at 02:40:12PM +0000, Stokes, Ian wrote: > > > On Thu, Nov 08, 2018 at 02:16:15PM +0000, Stokes, Ian wrote: > > > > > On 06.11.2018 17:31, Stokes, Ian wrote: > > > > > >> On 18.10.2018 16:29, Ilya Maximets wrote: > > > > > >>> vhost ports are not DPDK eth ports and has no rte_flow API. > > > > > >>> Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid > > > > > >>> time wasting and errors in log. > > > > > >>> > > > > > >>> Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c > > > > > >>> file, because there is no need to expose it in header. > > > > > >>> > > > > > >>> CC: Finn Christensen <fc@napatech.com> > > > > > >>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload > > > > > >>> with rte > > > > > >>> flow") > > > > > >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com> > > > > > >>> --- > > > > > >> > > > > > >> Hi Ian, > > > > > >> You didn't backport this patch to 2.10. Do you think that > > > > > >> it's not needed or you just missed it while preparing the pull > request? > > > > > >> > > > > > >> Periodic errors in log are a bit annoying. > > > > > > > > > > > > Hi Ilya, > > > > > > > > > > > > The patch above assumes that a previous commit 89c09c1cd1f0 > > > ("netdev: > > > > > Clean up class initialization.") is in place, however > > > > > 89c09c1cd1f0 > > > > > ("netdev: Clean up class initialization.") was never backported > > > > > to branch > > > > > 2.10 I had discussed this with Ben but we didn’t see the need. > > > > > > > > > > > > > > > > OK. I see. > > > > > > > > > > > As such the patch does not apply as the netdev dpdk class > > > > > > layout > > > > > differs. You could submit a specific patch for branch 2.10 with > > > > > an amended commit message. > > > > > > > > > > > > Alternatively I'm thinking it might make sense to backport > > > > > > 89c09c1cd1f0 > > > > > as well as the patch above in order to remove the periodic log > > > requests? > > > > > > > > > > In this case you'll need to backport also commit > > > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > > > > NO_OFFLOAD_API."). > > > > > > > > > > Maybe we can backport only changes related to netdev-dpdk like > this: > > > > > git cherry-pick -n 89c09c1cd1f0 c0af6425d \ > > > > > && git reset HEAD \ > > > > > && git add lib/netdev-dpdk.c \ > > > > > && git checkout . \ > > > > > && git commit -sv > > > > > > > > > > What do you think? > > > > > > > > Sure, that would work, I'd like to ask Bens opinion here also. > > > > > > > > Ben would above be ok or is it preferred to backport commits > > > > separately to 2.10 as > > > > > > > > 89c09c1cd1f0 ("netdev: Clean up class initialization.") > > > > 72713c651 ("netdev-bsd: Fix build failure because of undefined > > > NO_OFFLOAD_API."). > > > > c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") > > > > > > > > Is there a preference as regards keeping the original commits and > > > > sign > > > off tags for the code changes when backporting rather than creating > > > a new commit that combines code changes. > > > > > > Usually we backport commit individually because it makes it easier > > > to figure out what was backported. Individual commits are easy to > > > see at a glance in the history, squashed commits take a closer look. > > > > Thanks for clarifying, in that case I'll backport the three commits > separately. > > > > A quick follow up, commit 89c09c1cd1f0 isn't specific to netdev-dpdk and > also is not a bug fix but it does help avoid periodic errors in the logs > for HWOL. As it doesn't add new functionality I would assume it's ok to > backport? > > Seems fine to me. Thanks for the clarification, I've backported the following to branch 2.10 89c09c1cd1f0 ("netdev: Clean up class initialization.") 72713c651 ("netdev-bsd: Fix build failure because of undefined NO_OFFLOAD_API."). c0af6425d7ed ("netdev-dpdk: Drop offload API for vhost ports.") Thanks Ian
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index f91aa27cd..7fe5eb087 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -4696,6 +4696,10 @@ netdev_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, ufid, rte_flow); } +#define DPDK_FLOW_OFFLOAD_API \ + .flow_put = netdev_dpdk_flow_put, \ + .flow_del = netdev_dpdk_flow_del + #define NETDEV_DPDK_CLASS_COMMON \ .is_pmd = true, \ .alloc = netdev_dpdk_alloc, \ @@ -4717,8 +4721,7 @@ netdev_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, .rxq_alloc = netdev_dpdk_rxq_alloc, \ .rxq_construct = netdev_dpdk_rxq_construct, \ .rxq_destruct = netdev_dpdk_rxq_destruct, \ - .rxq_dealloc = netdev_dpdk_rxq_dealloc, \ - DPDK_FLOW_OFFLOAD_API + .rxq_dealloc = netdev_dpdk_rxq_dealloc #define NETDEV_DPDK_CLASS_BASE \ NETDEV_DPDK_CLASS_COMMON, \ @@ -4731,7 +4734,8 @@ netdev_dpdk_flow_del(struct netdev *netdev, const ovs_u128 *ufid, .get_features = netdev_dpdk_get_features, \ .get_status = netdev_dpdk_get_status, \ .reconfigure = netdev_dpdk_reconfigure, \ - .rxq_recv = netdev_dpdk_rxq_recv + .rxq_recv = netdev_dpdk_rxq_recv, \ + DPDK_FLOW_OFFLOAD_API static const struct netdev_class dpdk_class = { .type = "dpdk", diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h index cc0501d68..b7d02a77d 100644 --- a/lib/netdev-dpdk.h +++ b/lib/netdev-dpdk.h @@ -25,10 +25,6 @@ struct dp_packet; #ifdef DPDK_NETDEV -#define DPDK_FLOW_OFFLOAD_API \ - .flow_put = netdev_dpdk_flow_put, \ - .flow_del = netdev_dpdk_flow_del - void netdev_dpdk_register(void); void free_dpdk_buf(struct dp_packet *);
vhost ports are not DPDK eth ports and has no rte_flow API. Stop calling this API with DPDK_ETH_PORT_ID_INVALID to avoid time wasting and errors in log. Additionally, DPDK_FLOW_OFFLOAD_API definition moved to .c file, because there is no need to expose it in header. CC: Finn Christensen <fc@napatech.com> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow") Signed-off-by: Ilya Maximets <i.maximets@samsung.com> --- lib/netdev-dpdk.c | 10 +++++++--- lib/netdev-dpdk.h | 4 ---- 2 files changed, 7 insertions(+), 7 deletions(-)