Re: [ovs-dev] [PATCH] lib: fix typo in fragment handling error message
On Tue, May 29, 2018 at 08:51:15PM +0200, Louis Peens wrote:
> The error message states that "not_first" is a valid selection
> for the ip_frag field, but looking at the structure that is defined
> this should say "not_later".
>
> Signed-off-by: Louis Peens
> Reviewed-by: Pieter Jansen van Vuuren
> Reviewed-by: Simon Horman
Thanks Louis.
Everyone else,
are there any objections to me applying this change?
> ---
> lib/meta-flow.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 8b8d174..db0abb3 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -2821,7 +2821,7 @@ mf_from_frag_string(const char *s, uint8_t *valuep,
> uint8_t *maskp)
> }
>
> return xasprintf("%s: unknown fragment type (valid types are \"no\", "
> - "\"yes\", \"first\", \"later\", \"not_first\"", s);
> + "\"yes\", \"first\", \"later\", \"not_later\"", s);
> }
>
> static char *
> --
> 2.7.4
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpctl: Properly reflect a rule's offloaded to HW state
On Thu, Jun 07, 2018 at 09:36:59AM +0300, Gavi Teitz wrote:
> Previously, any rule that is offloaded via a netdev, not necessarily
> to the HW, would be reported as "offloaded". This patch fixes this
> misalignment, and introduces the 'dp' state, as follows:
>
> rule is in HW via TC offload -> offloaded=yes dp:tc
> rule is in not HW over TC DP -> offloaded=no dp:tc
> rule is in not HW over OVS DP -> offloaded=no dp:ovs
>
> To achieve this, the flows's 'offloaded' flag was encapsulated in a new
> attrs struct, which contains the offloaded state of the flow and the
> DP layer the flow is handled in, and instead of setting the flow's
> 'offloaded' state based solely on the type of dump it was acquired
> via, for netdev flows it now sends the new attrs struct to be
> collected along with the rest of the flow via the netdev, allowing
> it to be set per flow.
>
> For TC offloads, the offloaded state is set based on the 'in_hw' and
> 'not_in_hw' flags received from the TC as part of the flower. If no
> such flag was received, due to lack of kernel support, it defaults
> to true.
Thanks for your patch, this seems quite nice.
>
> Signed-off-by: Gavi Teitz
> Acked-by: Roi Dayan
...
> diff --git a/lib/dpctl.man b/lib/dpctl.man
> index 50623c4..7cf2087 100644
> --- a/lib/dpctl.man
> +++ b/lib/dpctl.man
> @@ -119,9 +119,9 @@ flow. As an example, \fBfilter='tcp,tp_src=100'\fR will
> match the
> datapath flow containing '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'.
> .IP
> If \fBtype=\fItype\fR is specified, only displays flows of a specific type.
> -\fItype\fR can be \fBoffloaded\fR to display only offloaded rules or
> \fBOVS\fR
> -to display only non-offloaded rules.
> -By default both offloaded and non-offloaded rules are displayed.
> +\fItype\fR can be \fBoffloaded\fR to display only rules offloaded to the HW
> +or \fBOVS\fR to display only rules from the OVS tables.
> +By default all rules are displayed.
This patch allows differentiation of flows based on the datapath (ovs or tc)
and offload (yes or no). But it seems that only the second is provided as
a filter by dpctl via the type described above. Perhaps it would be useful
to also expose the former?
> .
> .IP "\*(DX\fBadd\-flow\fR [\fIdp\fR] \fIflow actions\fR"
> .TP
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 1bd10a6..aa9bbd9 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1463,13 +1463,13 @@ dpif_netlink_init_flow_del(struct dpif_netlink *dpif,
> }
>
> enum {
> -DUMP_OVS_FLOWS_BIT = 0,
> -DUMP_OFFLOADED_FLOWS_BIT = 1,
> +DUMP_OVS_FLOWS_BIT= 0,
> +DUMP_NETDEV_FLOWS_BIT = 1,
> };
>
> enum {
> -DUMP_OVS_FLOWS = (1 << DUMP_OVS_FLOWS_BIT),
> -DUMP_OFFLOADED_FLOWS = (1 << DUMP_OFFLOADED_FLOWS_BIT),
> +DUMP_OVS_FLOWS= (1 << DUMP_OVS_FLOWS_BIT),
> +DUMP_NETDEV_FLOWS = (1 << DUMP_NETDEV_FLOWS_BIT),
Would TC make more sense than NETDEV here?
> };
>
> struct dpif_netlink_flow_dump {
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] lib: fix typo in fragment handling error message
On Wed, Jun 06, 2018 at 07:41:08AM -0700, Ben Pfaff wrote: > On Wed, Jun 06, 2018 at 11:47:13AM +0200, Simon Horman wrote: > > On Tue, May 29, 2018 at 08:51:15PM +0200, Louis Peens wrote: > > > The error message states that "not_first" is a valid selection > > > for the ip_frag field, but looking at the structure that is defined > > > this should say "not_later". > > > > > > Signed-off-by: Louis Peens > > > Reviewed-by: Pieter Jansen van Vuuren > > > > > > Reviewed-by: Simon Horman > > > > Thanks Louis. > > > > Everyone else, > > > > are there any objections to me applying this change? > > Acked-by: Ben Pfaff Thanks, pushed to master and branch-2.[5-9] ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpctl: Properly reflect a rule's offloaded to HW state
On Sun, Jun 10, 2018 at 03:05:39PM +, Gavi Teitz wrote:
> From: Simon Horman, Sent: Thursday, June 7, 2018 3:13 PM
> >On Thu, Jun 07, 2018 at 09:36:59AM +0300, Gavi Teitz wrote:
> >> Previously, any rule that is offloaded via a netdev, not necessarily
> >> to the HW, would be reported as "offloaded". This patch fixes this
> >> misalignment, and introduces the 'dp' state, as follows:
> >>
> >> rule is in HW via TC offload -> offloaded=yes dp:tc rule is in not HW
> >> over TC DP -> offloaded=no dp:tc rule is in not HW over OVS DP ->
> >> offloaded=no dp:ovs
> >>
> >> To achieve this, the flows's 'offloaded' flag was encapsulated in a
> >> new attrs struct, which contains the offloaded state of the flow and
> >> the DP layer the flow is handled in, and instead of setting the flow's
> >> 'offloaded' state based solely on the type of dump it was acquired
> >> via, for netdev flows it now sends the new attrs struct to be
> >> collected along with the rest of the flow via the netdev, allowing it
> >> to be set per flow.
> >>
> >> For TC offloads, the offloaded state is set based on the 'in_hw' and
> >> 'not_in_hw' flags received from the TC as part of the flower. If no
> >> such flag was received, due to lack of kernel support, it defaults to
> >> true.
> >
> >Thanks for your patch, this seems quite nice.
> >
> >>
> >> Signed-off-by: Gavi Teitz
> >> Acked-by: Roi Dayan
> >
> >...
> >
> >> diff --git a/lib/dpctl.man b/lib/dpctl.man index 50623c4..7cf2087
> >> 100644
> >> --- a/lib/dpctl.man
> >> +++ b/lib/dpctl.man
> >> @@ -119,9 +119,9 @@ flow. As an example, \fBfilter='tcp,tp_src=100'\fR
> >> will match the datapath flow containing
> >> '\fBtcp(src=80/0xff00,dst=8080/0xff)\fR'.
> >> .IP
> >> If \fBtype=\fItype\fR is specified, only displays flows of a specific
> >> type.
> >> -\fItype\fR can be \fBoffloaded\fR to display only offloaded rules or
> >> \fBOVS\fR -to display only non-offloaded rules.
> >> -By default both offloaded and non-offloaded rules are displayed.
> >> +\fItype\fR can be \fBoffloaded\fR to display only rules offloaded to
> >> +the HW or \fBOVS\fR to display only rules from the OVS tables.
> >> +By default all rules are displayed.
> >
> >This patch allows differentiation of flows based on the datapath (ovs or tc)
> >and offload (yes or no). But it seems that only the second is provided as a
> >filter by dpctl via the type described above. Perhaps it would be useful to
> >also expose the former?
> >
>
> This is something we would like to add, though we want to do it in a
> later commit so as not to delay the integration of this change. Also, I
> think that since we now can differentiate between flows based on the
> offloaded/not offloaded state and based on the datapath (ovs/tc/future
> implementation of a different datapath), we should be changing the
> current filter, which mixes the two, and have two filters, one of the
> offloaded state, and one of the datapath. Do you think that doing so
> would compromise backwards compatibility?
I think it should be possible to arrange things so that there is backwards
compatibility.
> >> .
> >> .IP "\*(DX\fBadd\-flow\fR [\fIdp\fR] \fIflow actions\fR"
> >> .TP
> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index
> >> 1bd10a6..aa9bbd9 100644
> >> --- a/lib/dpif-netlink.c
> >> +++ b/lib/dpif-netlink.c
> >> @@ -1463,13 +1463,13 @@ dpif_netlink_init_flow_del(struct dpif_netlink
> >> *dpif, }
> >>
> >> enum {
> >> -DUMP_OVS_FLOWS_BIT = 0,
> >> -DUMP_OFFLOADED_FLOWS_BIT = 1,
> >> +DUMP_OVS_FLOWS_BIT= 0,
> >> +DUMP_NETDEV_FLOWS_BIT = 1,
> >> };
> >>
> >> enum {
> >> -DUMP_OVS_FLOWS = (1 << DUMP_OVS_FLOWS_BIT),
> >> -DUMP_OFFLOADED_FLOWS = (1 << DUMP_OFFLOADED_FLOWS_BIT),
> >> +DUMP_OVS_FLOWS= (1 << DUMP_OVS_FLOWS_BIT),
> >> +DUMP_NETDEV_FLOWS = (1 << DUMP_NETDEV_FLOWS_BIT),
> >
> >Would TC make more sense than NETDEV here?
>
> Probably not, since TC isn't a known name in this context, but rather the
> offload channel is generically named netdev.
Understood.
I have gone ahead and applied the patch to master.
There was a minor conflict when applying the manpage hunk.
Please send an incremental patch to fix things if I got that wrong.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/6] offload Linux LAG devices to the TC datapath
On 21 June 2018 at 18:15, Ben Pfaff wrote: > Simon, I see that you have reviewed these patches. Are you handling > this series? > Hi Ben, yes, I reviewed these patches during internal review. I'm happy to handle the series now its in external review. > On Thu, Jun 21, 2018 at 02:35:55PM +0100, John Hurley wrote: > > This patchset extends OvS TC and the linux-netdev implementation to > > support the offloading of Linux Link Aggregation devices (LAG) and their > > slaves. TC blocks are used to provide this offload. Blocks, in TC, group > > together a series of qdiscs. If a filter is added to one of these qdiscs > > then it applied to all. Similarly, if a packet is matched on one of the > > grouped qdiscs then the stats for the entire block are increased. The > > basis of the LAG offload is that the LAG master (attached to the OvS > > bridge) and slaves that may exist outside of OvS are all added to the > same > > TC block. OvS can then control the filters and collect the stats on the > > slaves via its interaction with the LAG master. > > > > The TC API is extended within OvS to allow the addition of a block id to > > ingress qdisc adds. Block ids are then assigned to each LAG master that > is > > attached to the OvS bridge. The linux netdev netlink socket is used to > > monitor slave devices. If a LAG slave is found whose master is on the > bridge > > then it is added to the same block as its master. If the underlying > slaves > > belong to an offloadable device then the Linux LAG device can be > offloaded > > to hardware. > > > > John Hurley (6): > > tc: allow offloading of block ids > > netdev-provider: add class op to get block_id > > rtnetlink: extend parser to include kind of master and slave > > netdev-linux: indicate if netdev is a LAG master > > netdev-linux: assign LAG devs to tc blocks > > netdev-linux: monitor and offload LAG slaves to TC > > > > lib/netdev-bsd.c | 3 +- > > lib/netdev-dpdk.c| 3 +- > > lib/netdev-dummy.c | 3 +- > > lib/netdev-linux.c | 128 ++ > ++--- > > lib/netdev-provider.h| 4 ++ > > lib/netdev-tc-offloads.c | 82 +++--- > > lib/netdev-vport.c | 3 +- > > lib/netdev.c | 10 > > lib/netdev.h | 1 + > > lib/rtnetlink.c | 43 > > lib/rtnetlink.h | 4 ++ > > lib/tc.c | 60 +++--- > > lib/tc.h | 12 ++--- > > 13 files changed, 305 insertions(+), 51 deletions(-) > > mode change 100644 => 100755 lib/rtnetlink.h > > > > -- > > 2.7.4 > > > > ___ > > dev mailing list > > d...@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/6] offload Linux LAG devices to the TC datapath
On Thu, Jun 21, 2018 at 04:31:35PM -0700, Ben Pfaff wrote: > On Thu, Jun 21, 2018 at 06:27:19PM +0200, Simon Horman wrote: > > On 21 June 2018 at 18:15, Ben Pfaff wrote: > > > > > Simon, I see that you have reviewed these patches. Are you handling > > > this series? > > > > > > > Hi Ben, > > > > yes, I reviewed these patches during internal review. > > I'm happy to handle the series now its in external review. > > OK. > > If you want a second look at any of them, please let me know what you'd > like me to look at. Hi Ben, I believe the patches are clean and low risk in the sense that they should not adversely affect any existing use-cases. The series starts of with some plumbing and the last patch pulls things together. I would be grateful if you could find time to look over the last patch, perhaps rolling back to earlier patches if you need more context. But I am also comfortable moving forwards without your review. I'll let things sit for a few more days to see how things progress. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 0/6] offload Linux LAG devices to the TC datapath
On Thu, Jun 28, 2018 at 05:03:01PM +0100, John Hurley wrote: > This patchset extends OvS TC and the linux-netdev implementation to > support the offloading of Linux Link Aggregation devices (LAG) and their > slaves. TC blocks are used to provide this offload. Blocks, in TC, group > together a series of qdiscs. If a filter is added to one of these qdiscs > then it is applied to all. Similarly, if a packet is matched on one of the > grouped qdiscs then the stats for the entire block are increased. The > basis of the LAG offload is that the LAG master (attached to the OvS > bridge) and slaves that may exist outside of OvS are all added to the same > TC block. OvS can then control the filters and collect the stats on the > slaves via its interaction with the LAG master. > > The TC API is extended within OvS to allow the addition of a block id to > ingress qdisc adds. Block ids are then assigned to each LAG master that is > attached to the OvS bridge. The linux netdev netlink socket is used to > monitor slave devices. If a LAG slave is found whose master is on the bridge > then it is added to the same block as its master. If the underlying slaves > belong to an offloadable device then the Linux LAG device can be offloaded > to hardware. Thanks John, Applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-tc-offloads: Fix probing multi mask per prio
On Mon, Jul 02, 2018 at 12:07:58PM +0300, Roi Dayan wrote: > When adding TC rules we save the prio so can reuse same prio > for same mask since different mask will have to use different prio. > The multi mask per prio probe broke this by using a prio but > get_prio_for_tc_flower() didn't know about it. > Also multi mask per prio support changes the hash calculation. > It's best the probe will add and del the ingress qdisc to have a clean start > after it. > > Signed-off-by: Roi Dayan > Acked-by: Paul Blakey Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpctl: Expand the flow dump type filter
On Sun, Jul 08, 2018 at 02:15:38PM +0300, Gavi Teitz wrote: > Added new types to the flow dump filter, and allowed multiple filter > types to be passed at once, as a comma separated list. The new types > added are: > * tc - specifies flows handled by the tc dp > * non-offloaded - specifies flows not offloaded to the HW > * all - specifies flows of all types > > The type list is now fully parsed by the dpctl, and a new struct was > added to dpif which enables dpctl to define which types of dumps to > provide, rather than passing the type string and having dpif parse it. > > Signed-off-by: Gavi Teitz > Acked-by: Roi Dayan Hi, this looks good to me. However, I wonder if it should wait until after the v2.10 soft-freeze is over. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 0/4] Add support to offload QinQ
On Tue, Jul 17, 2018 at 02:01:53AM +, Jianbo Liu wrote: > This patchset is to support QinQ offloading, as TC flower supports QinQ. > > v3: > fix checkpatch warning in patch 4. > v2: > fix compile issue in patch 3. > > Jianbo Liu (4): > tc: Add VLAN tpid for push action > netdev-tc-offloads: Add support to match on 802.1AD ethertype > flow: Refactor some of VLAN helper functions > Add support to offload QinQ double VLAN headers match Thanks Jianbo, this looks fine to me but I think I should wait until the soft-freeze of the OVS tree to finish before applying this series. Reviewed-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpctl: Expand the flow dump type filter
On Thu, Jul 12, 2018 at 09:46:40AM +0300, Roi Dayan wrote: > > > On 11/07/2018 16:24, Simon Horman wrote: > > On Sun, Jul 08, 2018 at 02:15:38PM +0300, Gavi Teitz wrote: > >> Added new types to the flow dump filter, and allowed multiple filter > >> types to be passed at once, as a comma separated list. The new types > >> added are: > >> * tc - specifies flows handled by the tc dp > >> * non-offloaded - specifies flows not offloaded to the HW > >> * all - specifies flows of all types > >> > >> The type list is now fully parsed by the dpctl, and a new struct was > >> added to dpif which enables dpctl to define which types of dumps to > >> provide, rather than passing the type string and having dpif parse it. > >> > >> Signed-off-by: Gavi Teitz > >> Acked-by: Roi Dayan > > > > Hi, > > > > this looks good to me. However, I wonder if it should wait until after the > > v2.10 soft-freeze is over. > > > > it could. > currently offload unit tests in the repo fail and needs an update > though they cannot be fixed because they use veth and test dump > using type=offloaded to verify rules in tc. > this patch adds the possibility again to check rules in tc. > then we can fix the offload unit tests. > besides this nothing is really broken. I believe the freeze is now over, I have applied this patch to the master branch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 0/4] Add support to offload QinQ
On Tue, Jul 24, 2018 at 10:15:27AM +0200, Simon Horman wrote: > On Tue, Jul 17, 2018 at 02:01:53AM +, Jianbo Liu wrote: > > This patchset is to support QinQ offloading, as TC flower supports QinQ. > > > > v3: > > fix checkpatch warning in patch 4. > > v2: > > fix compile issue in patch 3. > > > > Jianbo Liu (4): > > tc: Add VLAN tpid for push action > > netdev-tc-offloads: Add support to match on 802.1AD ethertype > > flow: Refactor some of VLAN helper functions > > Add support to offload QinQ double VLAN headers match > > Thanks Jianbo, > > this looks fine to me but I think I should wait until the soft-freeze > of the OVS tree to finish before applying this series. > > Reviewed-by: Simon Horman I believe the freeze is now over, I have applied this series to the master branch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] Revert "dpctl: Expand the flow dump type filter"
Hi,
On 25 July 2018 at 23:17, Justin Pettit wrote:
>
> > On Jul 25, 2018, at 2:11 PM, Ben Pfaff wrote:
> >
> > On Wed, Jul 25, 2018 at 02:01:32PM -0700, Justin Pettit wrote:
> >> Commit ab15e70eb587 ("dpctl: Expand the flow dump type filter") had a
> >> number of issues with style, build breakage, and failing unit tests.
> >> The patch is being reverted so that they can addressed.
> >>
> >> This reverts commit ab15e70eb5878b46f8f84da940ffc915b6d74cad.
> >>
> >> CC: Gavi Teitz
> >> CC: Simon Horman
> >> CC: Roi Dayan
> >> CC: Aaron Conole
> >> Signed-off-by: Justin Pettit
> >
> > For the series:
> >Acked-by: Ben Pfaff
> > since it breaks unit tests and it isn't obvious how to fix them.
>
> Thanks.
>
> > But I suggest giving Simon and the others a chance to respond.
>
> Okay.
>
> > By the way, when you have CC: in a commit message, you're supposed to
> > actually CC those people in the email. "git send-email" does that
> > automatically for me but it looks like it didn't pick them up for you.
>
> Hmm. My "git send-email" says that they were all cc'd in the output of
> the command, but my mail client only shows Simon. I don't know where, or
> if, there's a problem, but it thinks it worked.
Sorry about this. I agree a revert was the correct course of action.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpctl: Expand the flow dump type filter
On 25 July 2018 at 23:13, Justin Pettit wrote: > > > On Jul 25, 2018, at 9:17 AM, Simon Horman > wrote: > > > > On Thu, Jul 12, 2018 at 09:46:40AM +0300, Roi Dayan wrote: > >> > >> it could. > >> currently offload unit tests in the repo fail and needs an update > >> though they cannot be fixed because they use veth and test dump > >> using type=offloaded to verify rules in tc. > >> this patch adds the possibility again to check rules in tc. > >> then we can fix the offload unit tests. > >> besides this nothing is really broken. > > > > I believe the freeze is now over, I have applied this patch to the master > > branch. > > Hi, Simon. This patch broke the build and a number of unit tests, so I > sent out a patch to revert it (and the subsequent build breakage patch): > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-July/ > 350128.html > > It appears that Roi indicated some unit tests were failing also (but it > sounds like those might be related to system unit tests as opposed to the > general ones). Are you running the unit tests before you commit code? > > On a less important note, it contains a number of lines that are over 79 > characters, which is discouraged by the coding style guide. > Sorry about this, I was not as thorough as I usually am before applying a patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/4] lib/tc: Handle ttl for ipv6 too
Hi Or,
On Wed, Jul 25, 2018 at 09:20:06PM +0300, Or Gerlitz wrote:
> TTL can and should be used to match on IPv6's hop-limit, fix that.
>
> Fixes: ab7ecf266b0a ('netdev-tc-offloads: Add nw_ttl matching using flower')
> Fixes: 0b4b5203d12e ('tc: Add ip layer ttl matching')
> Signed-off-by: Or Gerlitz
> Reviewed-by: Roi Dayan
> ---
> lib/netdev-tc-offloads.c | 4 ++--
> lib/tc.c | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 04548c7..2a6dd6d 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -1025,8 +1025,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> if (is_ip_any(key)) {
> flower.key.ip_proto = key->nw_proto;
> flower.mask.ip_proto = mask->nw_proto;
> +mask->nw_proto = 0;
> flower.key.ip_ttl = key->nw_ttl;
> flower.mask.ip_ttl = mask->nw_ttl;
> +mask->nw_ttl = 0;
>
> if (mask->nw_frag & FLOW_NW_FRAG_ANY) {
> flower.mask.flags |= TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT;
> @@ -1073,8 +1075,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> }
>
> mask->nw_tos = 0;
> -mask->nw_proto = 0;
> -mask->nw_ttl = 0;
I'm not sure that I understand the purpose of the changes above.
They seem to shuffle setting two mask values from one place to another.
But what is the effect of this?
>
> if (key->dl_type == htons(ETH_P_IP)) {
> flower.key.ipv4.ipv4_src = key->nw_src;
> diff --git a/lib/tc.c b/lib/tc.c
> index 2157135..f3fb59c 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1625,6 +1625,8 @@ nl_msg_put_flower_options(struct ofpbuf *request,
> struct tc_flower *flower)
> FLOWER_PUT_MASKED_VALUE(src_mac, TCA_FLOWER_KEY_ETH_SRC);
>
> if (host_eth_type == ETH_P_IP || host_eth_type == ETH_P_IPV6) {
> +FLOWER_PUT_MASKED_VALUE(ip_ttl, TCA_FLOWER_KEY_IP_TTL);
> +
> if (flower->mask.ip_proto && flower->key.ip_proto) {
> nl_msg_put_u8(request, TCA_FLOWER_KEY_IP_PROTO,
>flower->key.ip_proto);
> @@ -1653,7 +1655,6 @@ nl_msg_put_flower_options(struct ofpbuf *request,
> struct tc_flower *flower)
> if (host_eth_type == ETH_P_IP) {
> FLOWER_PUT_MASKED_VALUE(ipv4.ipv4_src, TCA_FLOWER_KEY_IPV4_SRC);
> FLOWER_PUT_MASKED_VALUE(ipv4.ipv4_dst, TCA_FLOWER_KEY_IPV4_DST);
> -FLOWER_PUT_MASKED_VALUE(ip_ttl, TCA_FLOWER_KEY_IP_TTL);
> } else if (host_eth_type == ETH_P_IPV6) {
> FLOWER_PUT_MASKED_VALUE(ipv6.ipv6_src, TCA_FLOWER_KEY_IPV6_SRC);
> FLOWER_PUT_MASKED_VALUE(ipv6.ipv6_dst, TCA_FLOWER_KEY_IPV6_DST);
> --
> 2.5.5
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/4] lib/tc: Support matching on ip tos
Hi Or,
On Wed, Jul 25, 2018 at 09:20:07PM +0300, Or Gerlitz wrote:
> Add the missing code to match on ip tos when dealing
> with the TC data-path.
>
> Signed-off-by: Or Gerlitz
> Reviewed-by: Roi Dayan
> ---
> include/openvswitch/match.h | 1 +
> lib/match.c | 7 +++
> lib/netdev-tc-offloads.c| 6 --
> lib/tc.c| 10 ++
> lib/tc.h| 1 +
> 5 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index b43ecb1..e8c80dd 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -194,6 +194,7 @@ void match_set_nw_dscp(struct match *, uint8_t);
> void match_set_nw_ecn(struct match *, uint8_t);
> void match_set_nw_ttl(struct match *, uint8_t nw_ttl);
> void match_set_nw_ttl_masked(struct match *, uint8_t nw_ttl, uint8_t mask);
> +void match_set_nw_tos_masked(struct match *, uint8_t nw_tos, uint8_t mask);
> void match_set_nw_frag(struct match *, uint8_t nw_frag);
> void match_set_nw_frag_masked(struct match *, uint8_t nw_frag, uint8_t mask);
> void match_set_icmp_type(struct match *, uint8_t);
> diff --git a/lib/match.c b/lib/match.c
> index 2281fa0..a1407a8 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -946,6 +946,13 @@ match_set_nw_ttl(struct match *match, uint8_t nw_ttl)
> }
>
> void
> +match_set_nw_tos_masked(struct match *match, uint8_t nw_tos, uint8_t mask)
> +{
> +match->flow.nw_tos = nw_tos & mask;
> +match->wc.masks.nw_tos = mask;
> +}
> +
> +void
> match_set_nw_ttl_masked(struct match *match, uint8_t nw_ttl, uint8_t mask)
> {
> match->flow.nw_ttl = nw_ttl & mask;
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 2a6dd6d..c61197a 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -455,6 +455,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> match_set_nw_proto(match, key->ip_proto);
> }
>
> +match_set_nw_tos_masked(match, key->ip_tos, mask->ip_tos);
> match_set_nw_ttl_masked(match, key->ip_ttl, mask->ip_ttl);
>
> if (mask->flags) {
> @@ -1026,6 +1027,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> flower.key.ip_proto = key->nw_proto;
> flower.mask.ip_proto = mask->nw_proto;
> mask->nw_proto = 0;
> +flower.key.ip_tos = key->nw_tos;
> +flower.mask.ip_tos = mask->nw_tos;
> +mask->nw_tos = 0;
> flower.key.ip_ttl = key->nw_ttl;
> flower.mask.ip_ttl = mask->nw_ttl;
> mask->nw_ttl = 0;
> @@ -1074,8 +1078,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> mask->tp_dst = 0;
> }
>
> -mask->nw_tos = 0;
> -
As per my comment on patch 1/2, the intention of the mask->nw_tos change
in the above two hunks is not clear to me.
> if (key->dl_type == htons(ETH_P_IP)) {
> flower.key.ipv4.ipv4_src = key->nw_src;
> flower.mask.ipv4.ipv4_src = mask->nw_src;
> diff --git a/lib/tc.c b/lib/tc.c
> index f3fb59c..8fbca7d 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -302,6 +302,10 @@ static const struct nl_policy tca_flower_policy[] = {
> .optional = true, },
> [TCA_FLOWER_KEY_IP_TTL_MASK] = { .type = NL_A_U8,
> .optional = true, },
> +[TCA_FLOWER_KEY_IP_TOS] = { .type = NL_A_U8,
> +.optional = true, },
> +[TCA_FLOWER_KEY_IP_TOS_MASK] = { .type = NL_A_U8,
> + .optional = true, },
> [TCA_FLOWER_KEY_TCP_FLAGS] = { .type = NL_A_U16,
> .optional = true, },
> [TCA_FLOWER_KEY_TCP_FLAGS_MASK] = { .type = NL_A_U16,
> @@ -497,6 +501,11 @@ nl_parse_flower_ip(struct nlattr **attrs, struct
> tc_flower *flower) {
> key->ip_ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TTL]);
> mask->ip_ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TTL_MASK]);
> }
> +
> +if (attrs[TCA_FLOWER_KEY_IP_TOS_MASK]) {
> +key->ip_tos = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TOS]);
> +mask->ip_tos = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TOS_MASK]);
> +}
> }
>
> static enum tc_offloaded_state
> @@ -1626,6 +1635,7 @@ nl_msg_put_flower_options(struct ofpbuf *request,
> struct tc_flower *flower)
>
> if (host_eth_type == ETH_P_IP || host_eth_type == ETH_P_IPV6) {
> FLOWER_PUT_MASKED_VALUE(ip_ttl, TCA_FLOWER_KEY_IP_TTL);
> +FLOWER_PUT_MASKED_VALUE(ip_tos, TCA_FLOWER_KEY_IP_TOS);
>
> if (flower->mask.ip_proto && flower->key.ip_proto) {
> nl_msg_put_u8(request, TCA_FLOWER_KEY_IP_PROTO,
> diff --git a/lib/tc.h b/lib/tc.h
> index 447d85f..90ef32a 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -95,6 +95,7 @@ struct tc_flower_key {
>
> uint8_t flags;
> uint8_t ip_ttl;
> +
Re: [ovs-dev] [PATCH 4/4] lib/tc: Support matching on ip tunnel tos and ttl
On Wed, Jul 25, 2018 at 09:20:09PM +0300, Or Gerlitz wrote:
> Support matching on tos and ttl of ip tunnels
> for the TC data-path.
>
> Signed-off-by: Or Gerlitz
> Reviewed-by: Roi Dayan
> ---
...
> diff --git a/lib/tc.c b/lib/tc.c
> index aa3147f..f5901d1 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -298,6 +298,10 @@ static const struct nl_policy tca_flower_policy[] = {
>.optional = true, },
> [TCA_FLOWER_KEY_FLAGS] = { .type = NL_A_BE32, .optional = true, },
> [TCA_FLOWER_KEY_FLAGS_MASK] = { .type = NL_A_BE32, .optional = true, },
> +[TCA_FLOWER_KEY_TCP_FLAGS] = { .type = NL_A_U16,
> + .optional = true, },
> +[TCA_FLOWER_KEY_TCP_FLAGS_MASK] = { .type = NL_A_U16,
> +.optional = true, },
> [TCA_FLOWER_KEY_IP_TTL] = { .type = NL_A_U8,
> .optional = true, },
> [TCA_FLOWER_KEY_IP_TTL_MASK] = { .type = NL_A_U8,
The above hunk appears to duplicate existing initialisation of the same
indexes of the array. Perhaps it should be dropped?
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/4] lib/tc: Handle ttl for ipv6 too
On Thu, Jul 26, 2018 at 05:54:51PM +0300, Or Gerlitz wrote:
> On Thu, Jul 26, 2018 at 5:41 PM, Simon Horman
> wrote:
> > On Wed, Jul 25, 2018 at 09:20:06PM +0300, Or Gerlitz wrote:
> >> TTL can and should be used to match on IPv6's hop-limit, fix that.
>
> >> Fixes: ab7ecf266b0a ('netdev-tc-offloads: Add nw_ttl matching using
> >> flower')
> >> Fixes: 0b4b5203d12e ('tc: Add ip layer ttl matching')
>
> >> --- a/lib/netdev-tc-offloads.c
> >> +++ b/lib/netdev-tc-offloads.c
> >> @@ -1025,8 +1025,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct
> >> match *match,
> >> if (is_ip_any(key)) {
> >> flower.key.ip_proto = key->nw_proto;
> >> flower.mask.ip_proto = mask->nw_proto;
> >> +mask->nw_proto = 0;
> >> flower.key.ip_ttl = key->nw_ttl;
> >> flower.mask.ip_ttl = mask->nw_ttl;
> >> +mask->nw_ttl = 0;
> >>
> >> if (mask->nw_frag & FLOW_NW_FRAG_ANY) {
> >> flower.mask.flags |= TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT;
> >> @@ -1073,8 +1075,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct
> >> match *match,
> >> }
> >>
> >> mask->nw_tos = 0;
> >> -mask->nw_proto = 0;
> >> -mask->nw_ttl = 0;
> >
> > I'm not sure that I understand the purpose of the changes above.
> > They seem to shuffle setting two mask values from one place to another.
> > But what is the effect of this?
>
> Setting mask->zzz to 0 means we consumed (== set into the mask
> of the tc rule) the zzz field. The convention in the code is to have
> this zeroing near the spot where you consume the field, I aligned
> this code to that convention while fixing the bug.
Understood, likewise for my similar comment on patch #2.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] Revert "dpctl: Expand the flow dump type filter"
Hi Gavi,
On 26 July 2018 at 17:36, Justin Pettit wrote:
>
> > On Jul 26, 2018, at 7:29 AM, Gavi Teitz wrote:
> >
> > From: Justin Pettit, sent: Thursday, July 26, 2018 12:02 AM:
> >> Commit ab15e70eb587 ("dpctl: Expand the flow dump type filter") had a
> number of issues with style, build breakage, and failing unit tests.
> >> The patch is being reverted so that they can addressed.
> >
> > I acknowledge the build breakage issue, could you elaborate regarding
> the style issues?
>
> The main style issue was that lines shouldn't be over 79 characters longs.
>
> > As for the failing unit tests, this commit provides the means to fix
> unit tests that lost their relevance due to the changes introduced in
> commit d63ca5329ff9 ("dpctl: Properly reflect a rule's offloaded to HW
> state"). Are there other unit tests that are broken due to this commit?
>
> I saw about a dozen unit tests failing when I ran "make check".
>
I believe this gives an overview of the failing tests:
https://travis-ci.org/horms2/ovs/jobs/408446254
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 3/3] ovn-northd: Propagate dynamic addresses to port group address sets.
Hi Jakub, On Mon, Jul 30, 2018 at 04:37:49PM +0200, Jakub Sitnicki wrote: > If a logical switch port belongs to a port group and has dynamic > addresses assigned, propagate the addresses to the auto-generated > address sets for the port group. > > Signed-off-by: Jakub Sitnicki > Acked-by: Han Zhou > --- > ovn/northd/ovn-northd.c | 34 > tests/ovn.at| 59 > + > 2 files changed, 84 insertions(+), 9 deletions(-) The test added by this patch, which is now present in the master branch, seems to fail. https://travis-ci.org/openvswitch/ovs/jobs/410550769 ... > diff --git a/tests/ovn.at b/tests/ovn.at > index e7bdfe250..32e6c1738 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -10267,6 +10267,65 @@ AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 > addresses], > > AT_CLEANUP > > +AT_SETUP([ovn -- Address Set generation from Port Groups (dynamic > addressing)]) > +ovn_start > + > +ovn-nbctl ls-add ls1 > +ovn-nbctl ls-add ls2 > +ovn-nbctl ls-add ls3 > + > +ovn-nbctl set Logical_Switch ls1 \ > +other_config:subnet=10.1.0.0/24 other_config:ipv6_prefix="2001:db8:1::" > +ovn-nbctl set Logical_Switch ls2 \ > +other_config:subnet=10.2.0.0/24 other_config:ipv6_prefix="2001:db8:2::" > +ovn-nbctl set Logical_Switch ls3 \ > +other_config:subnet=10.3.0.0/24 other_config:ipv6_prefix="2001:db8:3::" > + > +ovn-nbctl lsp-add ls1 lp1 > +ovn-nbctl lsp-add ls2 lp2 > +ovn-nbctl lsp-add ls3 lp3 > + > +ovn-nbctl lsp-set-addresses lp1 "02:00:00:00:00:01 dynamic" > +ovn-nbctl lsp-set-addresses lp2 "02:00:00:00:00:02 dynamic" > +ovn-nbctl lsp-set-addresses lp3 "02:00:00:00:00:03 dynamic" > + > +ovn-nbctl create Port_Group name=pg1 > +ovn-nbctl create Port_Group name=pg2 > + > +ovn-nbctl --id=@p get Logical_Switch_Port lp1 -- add Port_Group pg1 ports @p > +ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg1 ports @p > +ovn-nbctl --id=@p get Logical_Switch_Port lp2 -- add Port_Group pg2 ports @p > +ovn-nbctl --id=@p get Logical_Switch_Port lp3 -- add Port_Group pg2 ports @p > + > +ovn-nbctl --wait=sb sync > + > +dnl Check if port group address sets were populated with ports' addresses > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses], > + [0], [[["10.1.0.2", "10.2.0.2"]] > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg2_ip4 addresses], > + [0], [[["10.2.0.2", "10.3.0.2"]] > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], > + [0], [[["2001:db8:1::ff:fe00:1", "2001:db8:2::ff:fe00:2"]] > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg2_ip6 addresses], > + [0], [[["2001:db8:2::ff:fe00:2", "2001:db8:3::ff:fe00:3"]] > +]) > + > +ovn-nbctl set Logical_Switch ls1 \ > +other_config:subnet=10.11.0.0/24 other_config:ipv6_prefix="2001:db8:11::" > + Empirically the following allows the test to pass in my local environment: > +dnl Check if updated address got propagated to the port group address sets > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses], > + [0], [[["10.11.0.2", "10.2.0.2"]] - s/10.11.0.2/10.1.0.2/ > +]) > +AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses], > + [0], [[["2001:db8:11::ff:fe00:1", "2001:db8:2::ff:fe00:2"]] - s/2001:db8:11::ff:fe00:1/2001:db8:1::ff:fe00:1/ > +]) > + > +AT_CLEANUP > + > AT_SETUP([ovn -- ACL conjunction]) > ovn_start > > -- > 2.14.4 > > ___ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 0/4] Enable set/match of tos/ttl for IP tunnels on TC data-path
Thanks Or, Thanks Ben,
On 1 August 2018 at 08:43, Or Gerlitz wrote:
> On Tue, Jul 31, 2018 at 1:40 PM, Or Gerlitz wrote:
> > This series comes to address the case to set (encap) and match (decap)
> > also the tos and ttl fields of TC based IP tunnels. This happens e.g
> > due to inherit setup of tunnel port for tos or due to specific OF rule.
> >
> > The series is rebased over Jianbo's patches for QinQ [1]
>
> FWIW - note that v2 was actually rebased to the master where Jianbo's work
> is already applied
>
I have also reviewed these patches, tested that travis-ci is happy with
everything when applied on top of
185b13f228ac ("ovn: Add Meter and Meter_Band tables to the NB and SB
databases."), which was the most recent
travis-ci-clean commit in the master branch yesterday, and Netronome has
performed some testing in the lab.
Overall I am happy with these patches and plan to apply them later today
after one final run through travis-ci after rebasing onto the current
master branch (which is not travis-ci-clean :( [See: "Re: [ovs-dev] [PATCH
v2 3/3] ovn-northd: Propagate dynamic addresses to port group address
sets."]).
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn: Fix typos in "ovn -- Address Set generation..." test.
On Tue, Jul 31, 2018 at 12:45:41PM -0700, Ben Pfaff wrote:
> These caused the test to fail.
>
> CC: Jakub Sitnicki
> Fixes: 984c7d5ea8fe ("ovn-northd: Propagate dynamic addresses to port group
> address sets.")
> Signed-off-by: Ben Pfaff
> ---
> tests/ovn.at | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 17c740aa2352..1b25b6e4d6b9 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -10361,10 +10361,10 @@ ovn-nbctl set Logical_Switch ls1 \
>
> dnl Check if updated address got propagated to the port group address sets
> AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses],
> - [0], [[["10.11.0.2", "10.2.0.2"]]
> + [0], [[["10.1.0.2", "10.2.0.2"]]
> ])
> AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses],
> - [0], [[["2001:db8:11::ff:fe00:1", "2001:db8:2::ff:fe00:2"]]
> + [0], [[["2001:db8:1::ff:fe00:1", "2001:db8:2::ff:fe00:2"]]
> ])
>
> AT_CLEANUP
Reviewed-by: Simon Horman
Tested-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 3/3] ovn-northd: Propagate dynamic addresses to port group address sets.
On Wed, Aug 01, 2018 at 11:44:58AM +0200, Jakub Sitnicki wrote: > Hi Simon, > > On Wed, 1 Aug 2018 11:23:30 +0200 > Simon Horman wrote: > > > Hi Jakub, > > > > On Mon, Jul 30, 2018 at 04:37:49PM +0200, Jakub Sitnicki wrote: > > > If a logical switch port belongs to a port group and has dynamic > > > addresses assigned, propagate the addresses to the auto-generated > > > address sets for the port group. > > > > > > Signed-off-by: Jakub Sitnicki > > > Acked-by: Han Zhou > > > --- > > > ovn/northd/ovn-northd.c | 34 > > > tests/ovn.at| 59 > > > + > > > 2 files changed, 84 insertions(+), 9 deletions(-) > > > > The test added by this patch, which is now present in the master branch, > > seems to fail. > > > > https://travis-ci.org/openvswitch/ovs/jobs/410550769 > > Thanks for the heads-up. These patches depend on another patch that is > still under review. Hence the failure. I've already notified Ben, who > he has proposed a fix-up similar to yours: > > https://mail.openvswitch.org/pipermail/ovs-dev/2018-August/350390.html Thanks Jakub, I see that now. The fix looks good :) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 0/4] Enable set/match of tos/ttl for IP tunnels on TC data-path
On 1 August 2018 at 11:31, Simon Horman wrote:
> Thanks Or, Thanks Ben,
>
> On 1 August 2018 at 08:43, Or Gerlitz wrote:
>
>> On Tue, Jul 31, 2018 at 1:40 PM, Or Gerlitz
>> wrote:
>> > This series comes to address the case to set (encap) and match (decap)
>> > also the tos and ttl fields of TC based IP tunnels. This happens e.g
>> > due to inherit setup of tunnel port for tos or due to specific OF rule.
>> >
>> > The series is rebased over Jianbo's patches for QinQ [1]
>>
>> FWIW - note that v2 was actually rebased to the master where Jianbo's work
>> is already applied
>>
>
> I have also reviewed these patches, tested that travis-ci is happy with
> everything when applied on top of
> 185b13f228ac ("ovn: Add Meter and Meter_Band tables to the NB and SB
> databases."), which was the most recent
> travis-ci-clean commit in the master branch yesterday, and Netronome has
> performed some testing in the lab.
>
> Overall I am happy with these patches and plan to apply them later today
> after one final run through travis-ci after rebasing onto the current
> master branch (which is not travis-ci-clean :( [See: "Re: [ovs-dev] [PATCH
> v2 3/3] ovn-northd: Propagate dynamic addresses to port group address
> sets."]).
>
Thanks again Or, I have applied this series to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 0/4] Enable set/match of tos/ttl for IP tunnels on TC data-path
Hi Or,
On 1 August 2018 at 13:21, Or Gerlitz wrote:
> On Wed, Aug 1, 2018 at 2:07 PM, Simon Horman
> wrote:
> > On 1 August 2018 at 11:31, Simon Horman
> wrote:
> >>
> >> Thanks Or, Thanks Ben,
> >>
> >> On 1 August 2018 at 08:43, Or Gerlitz wrote:
> >>>
> >>> On Tue, Jul 31, 2018 at 1:40 PM, Or Gerlitz
> >>> wrote:
> >>> > This series comes to address the case to set (encap) and match
> (decap)
> >>> > also the tos and ttl fields of TC based IP tunnels. This happens e.g
> >>> > due to inherit setup of tunnel port for tos or due to specific OF
> rule.
> >>> >
> >>> > The series is rebased over Jianbo's patches for QinQ [1]
> >>>
> >>> FWIW - note that v2 was actually rebased to the master where Jianbo's
> >>> work
> >>> is already applied
> >>
> >>
> >> I have also reviewed these patches, tested that travis-ci is happy with
> >> everything when applied on top of
> >> 185b13f228ac ("ovn: Add Meter and Meter_Band tables to the NB and SB
> >> databases."), which was the most recent
> >> travis-ci-clean commit in the master branch yesterday, and Netronome has
> >> performed some testing in the lab.
> >>
> >> Overall I am happy with these patches and plan to apply them later today
> >> after one final run through travis-ci after rebasing onto the current
> master
> >> branch (which is not travis-ci-clean :( [See: "Re: [ovs-dev] [PATCH v2
> 3/3]
> >> ovn-northd: Propagate dynamic addresses to port group address sets."]).
>
> > Thanks again Or, I have applied this series to master.
>
>
> Thank you.
>
> So how is the stable process @ ovs goes? is that documented, where?
> e.g b4496fc "lib/tc: Handle ttl for ipv6 too" is a bug fix, should/who I
> ask
> for stable inclusion?
>
Hi Or,
The usual procedure, as I understand, is to ask if the maintainer doesn't
apply
the fix to the desired stable branches. I'll take the above as a request to
apply the patch to branch-2.10.
Do you want it considered for any other stable branches?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ACL Meters 4/7] ovn: Add Meter and Meter_Band tables to the NB and SB databases.
On Sun, Jul 29, 2018 at 11:46:35PM -0700, Justin Pettit wrote:
> Add support for configuring meters through the Meter and Meter_Band
> tables in the Northbound database. This commit also has ovn-northd
> sync those tables between the Northbound and Southbound databases.
>
> Add support for configuring meters with ovn-nbctl.
>
> Signed-off-by: Justin Pettit
Hi Justin,
it seems that this patch broke building with older GCC:
https://travis-ci.org/openvswitch/ovs/jobs/410404752:
Ben applied a fix for that to master.
04a12e42e089 ("ofctrl: Placate GCC.")
I believe that change is also needed in branch-2.10.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn: Fix typos in "ovn -- Address Set generation..." test.
On Wed, Aug 01, 2018 at 11:56:09AM +0200, Simon Horman wrote:
> On Tue, Jul 31, 2018 at 12:45:41PM -0700, Ben Pfaff wrote:
> > These caused the test to fail.
> >
> > CC: Jakub Sitnicki
> > Fixes: 984c7d5ea8fe ("ovn-northd: Propagate dynamic addresses to port group
> > address sets.")
> > Signed-off-by: Ben Pfaff
> > ---
> > tests/ovn.at | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 17c740aa2352..1b25b6e4d6b9 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -10361,10 +10361,10 @@ ovn-nbctl set Logical_Switch ls1 \
> >
> > dnl Check if updated address got propagated to the port group address sets
> > AT_CHECK([ovn-sbctl get Address_Set pg1_ip4 addresses],
> > - [0], [[["10.11.0.2", "10.2.0.2"]]
> > + [0], [[["10.1.0.2", "10.2.0.2"]]
> > ])
> > AT_CHECK([ovn-sbctl get Address_Set pg1_ip6 addresses],
> > - [0], [[["2001:db8:11::ff:fe00:1", "2001:db8:2::ff:fe00:2"]]
> > + [0], [[["2001:db8:1::ff:fe00:1", "2001:db8:2::ff:fe00:2"]]
> > ])
> >
> > AT_CLEANUP
>
> Reviewed-by: Simon Horman
> Tested-by: Simon Horman
I took the liberty of pushing this to master and branch-2.10
as I believe it allows those branches to pass all tests
covered by travis-ci.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 0/4] Enable set/match of tos/ttl for IP tunnels on TC data-path
On Wed, Aug 01, 2018 at 04:28:03PM +0300, Or Gerlitz wrote:
> On Wed, Aug 1, 2018 at 2:29 PM, Simon Horman
> wrote:
> > Hi Or,
> >
> > On 1 August 2018 at 13:21, Or Gerlitz wrote:
> >>
> >> On Wed, Aug 1, 2018 at 2:07 PM, Simon Horman
> >> wrote:
> >> > On 1 August 2018 at 11:31, Simon Horman
> >> > wrote:
> >> >>
> >> >> Thanks Or, Thanks Ben,
> >> >>
> >> >> On 1 August 2018 at 08:43, Or Gerlitz wrote:
> >> >>>
> >> >>> On Tue, Jul 31, 2018 at 1:40 PM, Or Gerlitz
> >> >>> wrote:
> >> >>> > This series comes to address the case to set (encap) and match
> >> >>> > (decap)
> >> >>> > also the tos and ttl fields of TC based IP tunnels. This happens e.g
> >> >>> > due to inherit setup of tunnel port for tos or due to specific OF
> >> >>> > rule.
> >> >>> >
> >> >>> > The series is rebased over Jianbo's patches for QinQ [1]
> >> >>>
> >> >>> FWIW - note that v2 was actually rebased to the master where Jianbo's
> >> >>> work
> >> >>> is already applied
> >> >>
> >> >>
> >> >> I have also reviewed these patches, tested that travis-ci is happy with
> >> >> everything when applied on top of
> >> >> 185b13f228ac ("ovn: Add Meter and Meter_Band tables to the NB and SB
> >> >> databases."), which was the most recent
> >> >> travis-ci-clean commit in the master branch yesterday, and Netronome
> >> >> has
> >> >> performed some testing in the lab.
> >> >>
> >> >> Overall I am happy with these patches and plan to apply them later
> >> >> today
> >> >> after one final run through travis-ci after rebasing onto the current
> >> >> master
> >> >> branch (which is not travis-ci-clean :( [See: "Re: [ovs-dev] [PATCH v2
> >> >> 3/3]
> >> >> ovn-northd: Propagate dynamic addresses to port group address sets."]).
> >>
> >> > Thanks again Or, I have applied this series to master.
> >>
> >>
> >> Thank you.
> >>
> >> So how is the stable process @ ovs goes? is that documented, where?
> >> e.g b4496fc "lib/tc: Handle ttl for ipv6 too" is a bug fix, should/who I
> >> ask
> >> for stable inclusion?
>
> > The usual procedure, as I understand, is to ask if the maintainer doesn't
> > apply
> > the fix to the desired stable branches. I'll take the above as a request to
> > apply the patch to branch-2.10.
> > Do you want it considered for any other stable branches?
>
> Hi Simon,
>
> Yes, please do apply the ttl fix to 2.10 and if possible, to 2.9 as well since
> the bug was introduced there.
>
> Also, it would be good if dfa2ccd "lib/tc: Support matching on ip tos"
> would also go to 2.10.
> I realized that commit 8f283af "netdev-tc-offloads: Implement netdev
> flow put using tc interface"
> has blindly set the tos field @ the mask to zero (see mask->nw_tos = 0
> in netdev_tc_flow_put)
> as if we offloaded that to the TC DP, but we didn't..
Thanks, I backported and pushed:
* to branch-2.9
- b4496fc "lib/tc: Handle ttl for ipv6 too"
* to branch-2.10
- b4496fc "lib/tc: Handle ttl for ipv6 too"
- dfa2ccd "lib/tc: Support matching on ip tos"
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 4/4] netdev-tc-offloads: Add support for action set
On Mon, Sep 25, 2017 at 05:48:49PM +0300, Paul Blakey wrote:
>
>
> On 18/09/2017 18:05, Simon Horman wrote:
> >On Mon, Sep 18, 2017 at 07:16:04AM +0300, Roi Dayan wrote:
> >>From: Paul Blakey
> >>
> >>Implement support for offloading ovs action set using
> >>tc header rewrite action.
> >>
> >>Signed-off-by: Paul Blakey
> >>Reviewed-by: Roi Dayan
> >>---
> >> lib/netdev-tc-offloads.c | 201
> >> +--
> >> 1 file changed, 195 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> >>index 3c145c2..4044a77 100644
> >>--- a/lib/netdev-tc-offloads.c
> >>+++ b/lib/netdev-tc-offloads.c
> >>@@ -27,11 +27,13 @@
> >> #include "openvswitch/ofpbuf.h"
> >> #include "openvswitch/thread.h"
> >> #include "openvswitch/types.h"
> >>+#include "openvswitch/util.h"
> >> #include "openvswitch/vlog.h"
> >> #include "netdev-linux.h"
> >> #include "netlink.h"
> >> #include "netlink-socket.h"
> >> #include "odp-netlink.h"
> >>+#include "odp-util.h"
> >> #include "tc.h"
> >> #include "unaligned.h"
> >> #include "util.h"
> >>@@ -41,6 +43,76 @@ VLOG_DEFINE_THIS_MODULE(netdev_tc_offloads);
> >> static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60, 5);
> >> static struct hmap ufid_tc = HMAP_INITIALIZER(&ufid_tc);
> >>+
> >>+struct netlink_field {
> >>+int offset;
> >>+int flower_offset;
> >>+int size;
> >>+};
> >>+
> >>+static struct netlink_field set_flower_map[][3] = {
> >>+[OVS_KEY_ATTR_IPV4] = {
> >>+{ offsetof(struct ovs_key_ipv4, ipv4_src),
> >>+ offsetof(struct tc_flower_key, ipv4.ipv4_src),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
> >>+},
> >>+{ offsetof(struct ovs_key_ipv4, ipv4_dst),
> >>+ offsetof(struct tc_flower_key, ipv4.ipv4_dst),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_dst)
> >>+},
> >>+{ offsetof(struct ovs_key_ipv4, ipv4_ttl),
> >>+ offsetof(struct tc_flower_key, ipv4.rewrite_ttl),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, ipv4.rewrite_ttl)
> >>+},
> >>+},
> >>+[OVS_KEY_ATTR_IPV6] = {
> >>+{ offsetof(struct ovs_key_ipv6, ipv6_src),
> >>+ offsetof(struct tc_flower_key, ipv6.ipv6_src),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_src)
> >>+},
> >>+{ offsetof(struct ovs_key_ipv6, ipv6_dst),
> >>+ offsetof(struct tc_flower_key, ipv6.ipv6_dst),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_dst)
> >>+},
> >>+},
> >>+[OVS_KEY_ATTR_ETHERNET] = {
> >>+{ offsetof(struct ovs_key_ethernet, eth_src),
> >>+ offsetof(struct tc_flower_key, src_mac),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, src_mac)
> >>+},
> >>+{ offsetof(struct ovs_key_ethernet, eth_dst),
> >>+ offsetof(struct tc_flower_key, dst_mac),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, dst_mac)
> >>+},
> >>+},
> >>+[OVS_KEY_ATTR_ETHERTYPE] = {
> >>+{ 0,
> >>+ offsetof(struct tc_flower_key, eth_type),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, eth_type)
> >>+},
> >>+},
> >>+[OVS_KEY_ATTR_TCP] = {
> >>+{ offsetof(struct ovs_key_tcp, tcp_src),
> >>+ offsetof(struct tc_flower_key, tcp_src),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, tcp_src)
> >>+},
> >>+{ offsetof(struct ovs_key_tcp, tcp_dst),
> >>+ offsetof(struct tc_flower_key, tcp_dst),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, tcp_dst)
> >>+},
> >>+},
> >>+[OVS_KEY_ATTR_UDP] = {
> >>+{ offsetof(struct ovs_key_udp, udp_src),
> >>+ offsetof(struct tc_flower_key, udp_src),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, udp_src)
> >>+},
> >>+{ offsetof(struct ovs_key_udp, udp_dst),
> >>+ offsetof(struct tc_flower_key, udp_dst),
> >>+ MEMBER_SIZEOF(struct tc_flower_key, udp_dst)
> >>+},
> >>+},
> >>+};
> >
> >Do you have any plans to add the following?
> >
> > OVS_KEY_ATTR_ICMP
> > OVS_KEY_ATTR_ICMPV6
> > OVS_KEY_ATTR_ARP
> > OVS_KEY_ATTR_ND
> > OVS_KEY_ATTR_SCTP
> > OVS_KEY_ATTR_TCP_FLAGS
> >
> >...
> >
>
>
> Yes, if pedit supports them (which I think it should). do you want it me to
> add it to this patchset, for us it would be faster to add those later after
> this is accepted as we have some inside testsuite for the currently
> supported ones and will need to come up with new ones for these.
Later is fine by me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action
On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
>
>
> On 18/09/2017 18:01, Simon Horman wrote:
> >On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> >>From: Paul Blakey
> >>
> >>To be later used to implement ovs action set offloading.
> >>
> >>Signed-off-by: Paul Blakey
> >>Reviewed-by: Roi Dayan
> >>---
> >> lib/tc.c | 372
> >> ++-
> >> lib/tc.h | 16 +++
> >> 2 files changed, 385 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/lib/tc.c b/lib/tc.c
> >>index c9cada2..743b2ee 100644
> >>--- a/lib/tc.c
> >>+++ b/lib/tc.c
> >>@@ -21,8 +21,10 @@
> >> #include
> >> #include
> >> #include
> >>+#include
> >> #include
> >> #include
> >>+#include
> >> #include
> >> #include
> >> #include
> >>@@ -33,11 +35,14 @@
> >> #include "netlink-socket.h"
> >> #include "netlink.h"
> >> #include "openvswitch/ofpbuf.h"
> >>+#include "openvswitch/util.h"
> >> #include "openvswitch/vlog.h"
> >> #include "packets.h"
> >> #include "timeval.h"
> >> #include "unaligned.h"
> >>+#define MAX_PEDIT_OFFSETS 8
> >
> >Why 8?
> We don't expect anything more right now (ipv6 src/dst rewrite requires 8
> pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> that's makes sens. do you suggest we increase it? to what?
It seems strange to me to place a somewhat arbitrary small limit
when none exists in the pedit API being used. I would at prefer if
it was at least a bigger, say 16 or 32.
> >> VLOG_DEFINE_THIS_MODULE(tc);
> >> static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60, 5);
> >>@@ -50,6 +55,82 @@ enum tc_offload_policy {
> >> static enum tc_offload_policy tc_policy = TC_POLICY_NONE;
> >>+struct tc_pedit_key_ex {
> >>+enum pedit_header_type htype;
> >>+enum pedit_cmd cmd;
> >>+};
> >>+
> >>+struct flower_key_to_pedit {
> >>+enum pedit_header_type htype;
> >>+int flower_offset;
> >>+int offset;
> >>+int size;
> >>+};
> >>+
> >>+static struct flower_key_to_pedit flower_pedit_map[] = {
> >>+{
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> >>+12,
> >>+offsetof(struct tc_flower_key, ipv4.ipv4_src),
> >>+MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> >>+16,
> >>+offsetof(struct tc_flower_key, ipv4.ipv4_dst),
> >>+MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_dst)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> >>+8,
> >>+offsetof(struct tc_flower_key, ipv4.rewrite_ttl),
> >>+MEMBER_SIZEOF(struct tc_flower_key, ipv4.rewrite_ttl)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_IP6,
> >>+8,
> >>+offsetof(struct tc_flower_key, ipv6.ipv6_src),
> >>+MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_src)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_IP6,
> >>+24,
> >>+offsetof(struct tc_flower_key, ipv6.ipv6_dst),
> >>+MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_dst)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
> >>+6,
> >>+offsetof(struct tc_flower_key, src_mac),
> >>+MEMBER_SIZEOF(struct tc_flower_key, src_mac)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
> >>+0,
> >>+offsetof(struct tc_flower_key, dst_mac),
> >>+MEMBER_SIZEOF(struct tc_flower_key, dst_mac)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
> >>+12,
> >>+offsetof(struct tc_flower_key, eth_type),
> >>+MEMBER_SIZEOF(struct tc_flower_key, eth_type)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_TCP,
> >>+0,
> >>+offsetof(struct tc_flower_key, tcp_src),
> >>+MEMBER_SIZEOF(struct tc_flower_key, tcp_src)
> >>+}, {
> >>+TCA_PEDIT_KEY_EX_HDR_TYPE_TCP,
> >>+2,
> >>+offsetof(str
Re: [ovs-dev] [PATCH] ovs-atomic: Reintroduce atomic_uint64_t and atomic_int64_t.
On Fri, Sep 29, 2017 at 10:18:52AM -0700, Ben Pfaff wrote:
> This is essentially a revert of commit e09d61c41b4f ("ovs-atomic: Remove
> atomic_uint64_t and atomic_int64_t.") My fear that some 32-bit platforms
> did not support 64-bit integers seems overblown, because OVS 2.6.x uses
> the 64-bit atomic_ullong and it is in Debian, which has tons of
> architectures.
>
> CC: Simon Horman
> Signed-off-by: Ben Pfaff
Hi Ben,
I do not have any recollection of the discussion around e09d61c41b4f,
but this seems reasonable to me.
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action
On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote:
>
>
> On 27/09/2017 12:08, Simon Horman wrote:
> > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
> > >
> > >
> > > On 18/09/2017 18:01, Simon Horman wrote:
> > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> > > > > From: Paul Blakey
> > > > >
> > > > > To be later used to implement ovs action set offloading.
> > > > >
> > > > > Signed-off-by: Paul Blakey
> > > > > Reviewed-by: Roi Dayan
> > > > > ---
> > > > > lib/tc.c | 372
> > > > > ++-
> > > > > lib/tc.h | 16 +++
> > > > > 2 files changed, 385 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > > index c9cada2..743b2ee 100644
> > > > > --- a/lib/tc.c
> > > > > +++ b/lib/tc.c
> > > > > @@ -21,8 +21,10 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -33,11 +35,14 @@
> > > > > #include "netlink-socket.h"
> > > > > #include "netlink.h"
> > > > > #include "openvswitch/ofpbuf.h"
> > > > > +#include "openvswitch/util.h"
> > > > > #include "openvswitch/vlog.h"
> > > > > #include "packets.h"
> > > > > #include "timeval.h"
> > > > > #include "unaligned.h"
> > > > > +#define MAX_PEDIT_OFFSETS 8
> > > >
> > > > Why 8?
> > > We don't expect anything more right now (ipv6 src/dst rewrite requires 8
> > > pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> > > that's makes sens. do you suggest we increase it? to what?
> >
> > It seems strange to me to place a somewhat arbitrary small limit
> > when none exists in the pedit API being used. I would at prefer if
> > it was at least a bigger, say 16 or 32.
>
>
> Hi Simon,
>
> Sorry for the late reply due to holidays and vacations.
> Me & Paul going to go over this and do the fixes needed and
> also rebase over latest master and run tests again.
Likewise, sorry for not responding earlier (same reason).
> I'll answer what I'm more familiar with now and Paul will continue.
> The 8 here is too low and you right. We used this definition
> for allocation of the pedit keys on the stack in
> nl_msg_put_flower_rewrite_pedits()
>
> It was for convenience instead of calculating the maximum possible
> keys that could exists and allocating it there and freeing it at
> the end.
>
> Increasing it to 32 is probably more than enough and wont waste much.
Thanks, that sounds good.
> > > > > VLOG_DEFINE_THIS_MODULE(tc);
> > > > > static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60,
> > > > > 5);
> > > > > @@ -50,6 +55,82 @@ enum tc_offload_policy {
> > > > > static enum tc_offload_policy tc_policy = TC_POLICY_NONE;
> > > > > +struct tc_pedit_key_ex {
> > > > > +enum pedit_header_type htype;
> > > > > +enum pedit_cmd cmd;
> > > > > +};
> > > > > +
> > > > > +struct flower_key_to_pedit {
> > > > > +enum pedit_header_type htype;
> > > > > +int flower_offset;
> > > > > +int offset;
> > > > > +int size;
> > > > > +};
> > > > > +
> > > > > +static struct flower_key_to_pedit flower_pedit_map[] = {
> > > > > +{
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> > > > > +12,
> > > > > +offsetof(struct tc_flower_key, ipv4.ipv4_src),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> > > > > +16,
> > > > > +offsetof(struct tc_flower_key, ipv4.ipv4_dst),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, i
Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action
On Tue, Oct 31, 2017 at 09:20:55AM +0200, Paul Blakey wrote:
>
>
> On 30/10/2017 15:42, Simon Horman wrote:
> > On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote:
> > >
> > >
> > > On 27/09/2017 12:08, Simon Horman wrote:
> > > > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
> > > > >
> > > > >
> > > > > On 18/09/2017 18:01, Simon Horman wrote:
> > > > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> > > > > > > From: Paul Blakey
> > > > > > >
> > > > > > > To be later used to implement ovs action set offloading.
> > > > > > >
> > > > > > > Signed-off-by: Paul Blakey
> > > > > > > Reviewed-by: Roi Dayan
> > > > > > > ---
> > > > > > >lib/tc.c | 372
> > > > > > > ++-
> > > > > > >lib/tc.h | 16 +++
> > > > > > >2 files changed, 385 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > > > > index c9cada2..743b2ee 100644
> > > > > > > --- a/lib/tc.c
> > > > > > > +++ b/lib/tc.c
> > > > > > > @@ -21,8 +21,10 @@
> > > > > > >#include
> > > > > > >#include
> > > > > > >#include
> > > > > > > +#include
> > > > > > >#include
> > > > > > >#include
> > > > > > > +#include
> > > > > > >#include
> > > > > > >#include
> > > > > > >#include
> > > > > > > @@ -33,11 +35,14 @@
> > > > > > >#include "netlink-socket.h"
> > > > > > >#include "netlink.h"
> > > > > > >#include "openvswitch/ofpbuf.h"
> > > > > > > +#include "openvswitch/util.h"
> > > > > > >#include "openvswitch/vlog.h"
> > > > > > >#include "packets.h"
> > > > > > >#include "timeval.h"
> > > > > > >#include "unaligned.h"
> > > > > > > +#define MAX_PEDIT_OFFSETS 8
> > > > > >
> > > > > > Why 8?
> > > > > We don't expect anything more right now (ipv6 src/dst rewrite
> > > > > requires 8
> > > > > pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> > > > > that's makes sens. do you suggest we increase it? to what?
> > > >
> > > > It seems strange to me to place a somewhat arbitrary small limit
> > > > when none exists in the pedit API being used. I would at prefer if
> > > > it was at least a bigger, say 16 or 32.
> > >
> > >
> > > Hi Simon,
> > >
> > > Sorry for the late reply due to holidays and vacations.
> > > Me & Paul going to go over this and do the fixes needed and
> > > also rebase over latest master and run tests again.
> >
> > Likewise, sorry for not responding earlier (same reason).
> >
> > > I'll answer what I'm more familiar with now and Paul will continue.
> > > The 8 here is too low and you right. We used this definition
> > > for allocation of the pedit keys on the stack in
> > > nl_msg_put_flower_rewrite_pedits()
> > >
> > > It was for convenience instead of calculating the maximum possible
> > > keys that could exists and allocating it there and freeing it at
> > > the end.
> > >
> > > Increasing it to 32 is probably more than enough and wont waste much.
> >
> > Thanks, that sounds good.
> >
> > > > > > >VLOG_DEFINE_THIS_MODULE(tc);
> > > > > > >static struct vlog_rate_limit error_rl =
> > > > > > > VLOG_RATE_LIMIT_INIT(60, 5);
> > > > > > > @@ -50,6 +55,82 @@ enum tc_offload_policy {
> > > > > > >static enum tc_offload_policy tc_policy = TC_POLICY_NONE;
> > > > > > > +struct tc_pedit_key_ex {
> > > > > > > +enum pedit_header_type htype;
> > > > > > > +enum pedit_cmd c
Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action
On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote:
>
>
> On 27/09/2017 12:08, Simon Horman wrote:
> > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
> > >
> > >
> > > On 18/09/2017 18:01, Simon Horman wrote:
> > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> > > > > From: Paul Blakey
> > > > >
> > > > > To be later used to implement ovs action set offloading.
> > > > >
> > > > > Signed-off-by: Paul Blakey
> > > > > Reviewed-by: Roi Dayan
> > > > > ---
> > > > > lib/tc.c | 372
> > > > > ++-
> > > > > lib/tc.h | 16 +++
> > > > > 2 files changed, 385 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > > index c9cada2..743b2ee 100644
> > > > > --- a/lib/tc.c
> > > > > +++ b/lib/tc.c
> > > > > @@ -21,8 +21,10 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > +#include
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > @@ -33,11 +35,14 @@
> > > > > #include "netlink-socket.h"
> > > > > #include "netlink.h"
> > > > > #include "openvswitch/ofpbuf.h"
> > > > > +#include "openvswitch/util.h"
> > > > > #include "openvswitch/vlog.h"
> > > > > #include "packets.h"
> > > > > #include "timeval.h"
> > > > > #include "unaligned.h"
> > > > > +#define MAX_PEDIT_OFFSETS 8
> > > >
> > > > Why 8?
> > > We don't expect anything more right now (ipv6 src/dst rewrite requires 8
> > > pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> > > that's makes sens. do you suggest we increase it? to what?
> >
> > It seems strange to me to place a somewhat arbitrary small limit
> > when none exists in the pedit API being used. I would at prefer if
> > it was at least a bigger, say 16 or 32.
>
> Hi Simon,
>
> Sorry for the late reply due to holidays and vacations.
> Me & Paul going to go over this and do the fixes needed and
> also rebase over latest master and run tests again.
>
> I'll answer what I'm more familiar with now and Paul will continue.
> The 8 here is too low and you right. We used this definition
> for allocation of the pedit keys on the stack in
> nl_msg_put_flower_rewrite_pedits()
>
> It was for convenience instead of calculating the maximum possible
> keys that could exists and allocating it there and freeing it at
> the end.
>
> Increasing it to 32 is probably more than enough and wont waste much.
I updated the value to 32 when applying the patch.
...
> > > > If I understand the above correctly it is designed to make
> > > > pedit actions disjoint. If so, why is that necessary? >
> > >
> > > It's not, as a single flower key rewrite can be split to multiple pedit
> > > actions it finds the overlap between a flower key and a pedit action, if
> > > they do overlap it translates it to the correct offset and masks it out.
> >
> > Thanks, understood.
> >
> > >
> > > > > +} else {
> > > > > +VLOG_ERR_RL(&error_rl, "unable to parse legacy pedit
> > > > > type: %d",
> > > > > +nl_attr_type(nla));
> > > > > +return EOPNOTSUPP;
> > > > > +}
> > > >
> > > > I think the code could exit early here as
> > > > nl_msg_put_flower_rewrite_pedits() does below.
> > > >
> > >
> > > Sorry, didn't understand. can you give an example?
> > >
> >
> > I meant something like this.
> >
> > if (nl_attr_type(nla) != TCA_PEDIT_KEY_EX) {
> > VLOG_ERR_RL(...);
> > return EOPNOTSUPP;
> > }
>
> understood. we'll do that. thanks.
I also fixed this when applying the patch.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 0/4] Add offload support for action set
On Mon, Sep 18, 2017 at 07:16:00AM +0300, Roi Dayan wrote: > Hi, > > This series adds support for offloading action set using > tc interface. > > V1->V2: > - Check patch whitespaces fixes > - Changed flower_pedit_map to be non sparse. > - Missing handling of unsupported attributes and sub > attributes of action set key struct > - Removed unnsessary loop over action SET/SET_MASKED >(has only one nested attr > > Thanks, > Roi Applied, thanks. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-tc-offloads: update stats properly on flow deletion
On Fri, Nov 17, 2017 at 04:45:05PM -0200, Flavio Leitner wrote:
> On Fri, 17 Nov 2017 14:36:22 +0100
> Paolo Abeni wrote:
> > Currently, when an offloaded DP flow is deleted, the related stats
> > are unconditionally cleared. As a result the counters for the
> > originating open flow are corrupted.
> >
> > This change addresses the issue updating the DP stats with the current
> > values provided by the flower APIs before deleting the tc filter, as
> > currently done by others DP providers.
> >
> > Tested vs different OVS H/W offload devices, verifying that the open flow
> > stats are correct after the expiration of the related, H/W offloaded DP
> > flow.
> >
> > Fixes: 30b6b047260b ("netdev-tc-offloads: Implement netdev flow del using
> > tc interface")
> > CC: Paul Blakey
> > Reported-by: Kumar Sanghvi
> > Signed-off-by: Paolo Abeni
> > ---
> > v1 -> v2:
> > - ensure to always clear stats if tc_get_flower() fails for some reasons
> > ---
> > lib/netdev-tc-offloads.c | 13 ++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> > index f86f3e046..781d849e4 100644
> > --- a/lib/netdev-tc-offloads.c
> > +++ b/lib/netdev-tc-offloads.c
> > @@ -1101,6 +1101,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> > const ovs_u128 *ufid,
> > struct dpif_flow_stats *stats)
> > {
> > +struct tc_flower flower;
> > struct netdev *dev;
> > int prio = 0;
> > int ifindex;
> > @@ -1120,14 +1121,20 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> > return -ifindex;
> > }
> >
> > +if (stats) {
> > +memset(stats, 0, sizeof *stats);
> > +if (!tc_get_flower(ifindex, prio, handle, &flower)) {
> > +stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
> > +stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
> > +stats->used = flower.lastused;
> > +}
> > +}
> > +
> > error = tc_del_filter(ifindex, prio, handle);
> > del_ufid_tc_mapping(ufid);
> >
> > netdev_close(dev);
> >
> > -if (stats) {
> > -memset(stats, 0, sizeof *stats);
> > -}
> > return error;
> > }
> >
>
> Thanks for the V2 :-)
> Acked-by: Flavio Leitner
Thanks, applied to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action
On Sun, Nov 19, 2017 at 08:45:19AM +0200, Roi Dayan wrote:
>
>
> On 16/11/2017 18:29, Simon Horman wrote:
> > On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote:
> > >
> > >
> > > On 27/09/2017 12:08, Simon Horman wrote:
> > > > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
> > > > >
> > > > >
> > > > > On 18/09/2017 18:01, Simon Horman wrote:
> > > > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> > > > > > > From: Paul Blakey
> > > > > > >
> > > > > > > To be later used to implement ovs action set offloading.
> > > > > > >
> > > > > > > Signed-off-by: Paul Blakey
> > > > > > > Reviewed-by: Roi Dayan
> > > > > > > ---
> > > > > > >lib/tc.c | 372
> > > > > > > ++-
> > > > > > >lib/tc.h | 16 +++
> > > > > > >2 files changed, 385 insertions(+), 3 deletions(-)
> > > > > > >
> > > > > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > > > > index c9cada2..743b2ee 100644
> > > > > > > --- a/lib/tc.c
> > > > > > > +++ b/lib/tc.c
> > > > > > > @@ -21,8 +21,10 @@
> > > > > > >#include
> > > > > > >#include
> > > > > > >#include
> > > > > > > +#include
> > > > > > >#include
> > > > > > >#include
> > > > > > > +#include
> > > > > > >#include
> > > > > > >#include
> > > > > > >#include
> > > > > > > @@ -33,11 +35,14 @@
> > > > > > >#include "netlink-socket.h"
> > > > > > >#include "netlink.h"
> > > > > > >#include "openvswitch/ofpbuf.h"
> > > > > > > +#include "openvswitch/util.h"
> > > > > > >#include "openvswitch/vlog.h"
> > > > > > >#include "packets.h"
> > > > > > >#include "timeval.h"
> > > > > > >#include "unaligned.h"
> > > > > > > +#define MAX_PEDIT_OFFSETS 8
> > > > > >
> > > > > > Why 8?
> > > > > We don't expect anything more right now (ipv6 src/dst rewrite
> > > > > requires 8
> > > > > pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> > > > > that's makes sens. do you suggest we increase it? to what?
> > > >
> > > > It seems strange to me to place a somewhat arbitrary small limit
> > > > when none exists in the pedit API being used. I would at prefer if
> > > > it was at least a bigger, say 16 or 32.
> > >
> >
> > > Hi Simon,
> > >
> > > Sorry for the late reply due to holidays and vacations.
> > > Me & Paul going to go over this and do the fixes needed and
> > > also rebase over latest master and run tests again.
> > >
> > > I'll answer what I'm more familiar with now and Paul will continue.
> > > The 8 here is too low and you right. We used this definition
> > > for allocation of the pedit keys on the stack in
> > > nl_msg_put_flower_rewrite_pedits()
> > >
> > > It was for convenience instead of calculating the maximum possible
> > > keys that could exists and allocating it there and freeing it at
> > > the end.
> > >
> > > Increasing it to 32 is probably more than enough and wont waste much.
> >
> > I updated the value to 32 when applying the patch.
> >
> > ...
> >
> > > > > > If I understand the above correctly it is designed to make
> > > > > > pedit actions disjoint. If so, why is that necessary? >
> > > > >
> > > > > It's not, as a single flower key rewrite can be split to multiple
> > > > > pedit
> > > > > actions it finds the overlap between a flower key and a pedit action,
> > > > > if
> > > > > they do overlap it translates it to the correct offset and masks it
> > > > > out.
> > > >
> > > > Thanks, understood.
> > > >
> > > > >
> > > > > > > +} else {
> > > > > > > +VLOG_ERR_RL(&error_rl, "unable to parse legacy pedit
> > > > > > > type: %d",
> > > > > > > +nl_attr_type(nla));
> > > > > > > +return EOPNOTSUPP;
> > > > > > > +}
> > > > > >
> > > > > > I think the code could exit early here as
> > > > > > nl_msg_put_flower_rewrite_pedits() does below.
> > > > > >
> > > > >
> > > > > Sorry, didn't understand. can you give an example?
> > > > >
> > > >
> > > > I meant something like this.
> > > >
> > > > if (nl_attr_type(nla) != TCA_PEDIT_KEY_EX) {
> > > > VLOG_ERR_RL(...);
> > > > return EOPNOTSUPP;
> > > > }
> > >
> > > understood. we'll do that. thanks.
> >
> > I also fixed this when applying the patch.
> >
> > ...
> >
>
>
> Thanks Simon. sorry we were not responsive enough with this feature.
> We'll improve that.
>
No problem.
Could you work on a fix for the travis failure that Eric Garver flagged?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action
On Tue, Nov 21, 2017 at 07:50:24AM +0200, Roi Dayan wrote: > > > On 20/11/2017 14:30, Simon Horman wrote: > > On Sun, Nov 19, 2017 at 08:45:19AM +0200, Roi Dayan wrote: > > > > > > > > > On 16/11/2017 18:29, Simon Horman wrote: > > > > On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote: > > > > > > > > > > > > > > > On 27/09/2017 12:08, Simon Horman wrote: > > > > > > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote: > > > > > > > > > > > > > > > > > > > > > On 18/09/2017 18:01, Simon Horman wrote: > > > > > > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote: > > > > > > > > > From: Paul Blakey > > > > > > > > > > > > > > > > > > To be later used to implement ovs action set offloading. > > > > > > > > > > > > > > > > > > Signed-off-by: Paul Blakey > > > > > > > > > Reviewed-by: Roi Dayan > > > > > > > > > --- > > > > > > > > > lib/tc.c | 372 > > > > > > > > > ++- > > > > > > > > > lib/tc.h | 16 +++ > > > > > > > > > 2 files changed, 385 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/lib/tc.c b/lib/tc.c > > > > > > > > > index c9cada2..743b2ee 100644 > > > > > > > > > --- a/lib/tc.c > > > > > > > > > +++ b/lib/tc.c > > > > > > > > > @@ -21,8 +21,10 @@ > > > > > > > > > #include > > > > > > > > > #include > > > > > > > > > #include > > > > > > > > > +#include > > > > > > > > > #include > > > > > > > > > #include > > > > > > > > > +#include > > > > > > > > > #include > > > > > > > > > #include > > > > > > > > > #include > > > > > > > > > @@ -33,11 +35,14 @@ > > > > > > > > > #include "netlink-socket.h" > > > > > > > > > #include "netlink.h" > > > > > > > > > #include "openvswitch/ofpbuf.h" > > > > > > > > > +#include "openvswitch/util.h" > > > > > > > > > #include "openvswitch/vlog.h" > > > > > > > > > #include "packets.h" > > > > > > > > > #include "timeval.h" > > > > > > > > > #include "unaligned.h" > > > > > > > > > +#define MAX_PEDIT_OFFSETS 8 > > > > > > > > > > > > > > > > Why 8? > > > > > > > We don't expect anything more right now (ipv6 src/dst rewrite > > > > > > > requires 8 > > > > > > > pedits iirc). I can't think of a larger use case, maybe ipv6 + > > > > > > > macs if > > > > > > > that's makes sens. do you suggest we increase it? to what? > > > > > > > > > > > > It seems strange to me to place a somewhat arbitrary small limit > > > > > > when none exists in the pedit API being used. I would at prefer if > > > > > > it was at least a bigger, say 16 or 32. > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > Sorry for the late reply due to holidays and vacations. > > > > > Me & Paul going to go over this and do the fixes needed and > > > > > also rebase over latest master and run tests again. > > > > > > > > > > I'll answer what I'm more familiar with now and Paul will continue. > > > > > The 8 here is too low and you right. We used this definition > > > > > for allocation of the pedit keys on the stack in > > > > > nl_msg_put_flower_rewrite_pedits() > > > > > > > > > > It was for convenience instead of calculating the maximum possible &g
Re: [ovs-dev] [PATCH 0/7] Fixes for header rewrite feature
On Tue, Nov 21, 2017 at 02:40:35PM +0200, Roi Dayan wrote: > Hi, > > The following series fixes some issues with header rewrite > and some small refactoring. Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 0/5] OVS-DPDK flow offload with rte_flow
On Wed, Jan 10, 2018 at 02:53:19PM +, Chandran, Sugesh wrote: > Hi All, > > Based on the comments below, I wanted to call out for a meeting to decide > what further needs to be done to handle different kinds of hardware > acceleration in OVS-DPDK. We have some POC implementation that partially > handles some of the cases , But it only work with some of Intel hardware > acceleration products. I also did a presentation about that proposal in the > last OVS conference. I feel its time to discuss now to come up with a design > that work for all different hardware acceleration devices and possible > acceleration modes. > If all are agree on the above suggestion, I can send out for a meeting invite > sometime next week.(May be Jan15th, Monday 5:00PM GMT). Let me know if this > timeslot is OK for everyone. > > I am ccing the 'dev' mailing list as well here in case if anyone else wanted > to join the call and provide the inputs. As per the brief discussion of this at the OVS-DPDK meeting on Wednesday I agree this is a good idea. Please include me on the invitation (or point me to the invitation if its already out). ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH OVS 1/1] dpif: geneve: supply dpif function to get ifindex
On 16 January 2018 at 11:46, John Hurley wrote: > > Geneve tunnels are not given a netdev_class function to determine their > ifindex. This means when ofproto-dpif attempts to add a geneve netdev > it fails in 'netdev_ports_insert' in netdev.c. Failure to add this means > that further operations like offloading a rule that egresses to a geneve > port will be rejected as the egress port cannot be found. This patch > applies the same ifindex function to geneve as is used in vxlan. > > Signed-off-by: John Hurley > Acked-by: Simon Horman Thanks John, I have applied this to master, branch-2.9 and branch-2.8. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH OVS 1/1] tc flower: reorder tunnel encap/decap actions
2018/01/23 19:02 "Ben Pfaff" :
Simon, do you want to apply this? I see that you reviewed it.
Sure, will do.
On Tue, Jan 23, 2018 at 02:08:42PM +, John Hurley wrote:
> The tc_flower conversion struct does not consider the order of actions.
> If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel,
> the netlink conversion to TC will add the set tunnel key action before the
> unset, leading to an incorrect TC rule. This patch reorders the netlink
> generation to ensure a decap is done before an encap if both exist.
>
> Signed-off-by: John Hurley
> Reviewed-by: Simon Horman
> ---
> lib/tc.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index bfe20ce..914465a 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1379,6 +1379,11 @@ nl_msg_put_flower_acts(struct ofpbuf *request,
struct tc_flower *flower)
> nl_msg_end_nested(request, act_offset);
> }
> }
> +if (flower->tunnel.tunnel) {
> +act_offset = nl_msg_start_nested(request, act_index++);
> +nl_msg_put_act_tunnel_key_release(request);
> +nl_msg_end_nested(request, act_offset);
> +}
> if (flower->set.set) {
> act_offset = nl_msg_start_nested(request, act_index++);
> nl_msg_put_act_tunnel_key_set(request, flower->set.id,
> @@ -1389,11 +1394,6 @@ nl_msg_put_flower_acts(struct ofpbuf *request,
struct tc_flower *flower)
>flower->set.tp_dst);
> nl_msg_end_nested(request, act_offset);
> }
> -if (flower->tunnel.tunnel) {
> -act_offset = nl_msg_start_nested(request, act_index++);
> -nl_msg_put_act_tunnel_key_release(request);
> -nl_msg_end_nested(request, act_offset);
> -}
> if (flower->vlan_pop) {
> act_offset = nl_msg_start_nested(request, act_index++);
> nl_msg_put_act_pop_vlan(request);
> --
> 1.9.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH OVS 1/1] tc flower: reorder tunnel encap/decap actions
On Tue, Jan 23, 2018 at 02:08:42PM +, John Hurley wrote: > The tc_flower conversion struct does not consider the order of actions. > If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel, > the netlink conversion to TC will add the set tunnel key action before the > unset, leading to an incorrect TC rule. This patch reorders the netlink > generation to ensure a decap is done before an encap if both exist. > > Signed-off-by: John Hurley > Reviewed-by: Simon Horman Thanks John, I have applied to master and branch-2.9. It looks like the fix is also needed for branch-2.8 but does not apply cleanly there. Please consider posting a backport to that branch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH branch-2.8 1/1] tc flower: reorder tunnel encap/decap actions
On Thu, Jan 25, 2018 at 03:23:21PM +, John Hurley wrote: > The tc_flower conversion struct does not consider the order of actions. > If an OvS rule matches on a tunnel (decap) and outputs to a new tunnel, > the netlink conversion to TC will add the set tunnel key action before the > unset, leading to an incorrect TC rule. This patch reorders the netlink > generation to ensure a decap is done before an encap if both exist. > > Patch was committed to master. Backport to branch 2.8 requested by Simon > Horman. > > Signed-off-by: John Hurley > Reviewed-by: Simon Horman Thanks John, applied to branch-2.8. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] Minutes: OvS Offload Discussion at Netdev 2.1
At Netdev 2.1 a meeting was held to discuss OvS offload. Minutes of the discussion follow. I apologise in advance for any errors or omissions; doubly for any errors in the attendee list. Topic: OVS Hardware Offload Using TC Date: 7th April 2017 Location: Netdev 2.1, Montreal Attendees: Aaron Conole, Ben LaHaise, Eran Ben Elisha, Hannes Frederic Sowa, Jakub Kicinski, Jiri Pirko, Joe Stringer, John Fastabend, Nick Viljoen, Rashid Khan, Rony Efraim, Simon Horman Joe raised 2 concerns: 1) How to enable users to understand whether offload is successful and if not, why not? a) There is functionality in the v7[1] patchset to report which flows are present in hardware. b) New error reporting infrastructure from the kernel is forthcoming It should allow TC to provide more error information if a flow can't be added to hardware. This could be made available to users - e.g. logged - to allow them better understand the reason for the failure. 2) Maintenance burden falling on existing maintainers a) Simon offered to take some of the maintenance burden immediately as he is already a committer. b) The aim is to ensure that in future there are other committers who are interested in this feature. There was consensus that if the feature-set does not grow there should be discussion of deprecating the HW offload support provided by [1]. Joe raised issue of whether OVS should probe hardware capabilities at runtime. John suggested this may be complex; potential combinatorial set is too large. Rony then raised the increased complexity of using multiple NICs of different types with different offload capabilities, this was tabled to a later date. Joe has expressed a desire for more testing. There was a general agreement to contribute tests. [1] [PATCH ovs V7 00/24] Introducing HW offload support for openvswitch ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Minutes: OvS Offload Discussion at Netdev 2.1
On Mon, Apr 10, 2017 at 09:59:57AM -0700, Joe Stringer wrote: > Thanks Simon. I also raised with a few people after the discussion about > how we should document this functionality​. Specifically, whether there > should be an 'experimental' or 'beta' tag associated. I believe that this > seemed reasonable to the people I discussed this with. Thanks Joe, FWIW that sounds reasonable to me. > On 8/04/2017 16:48, "Simon Horman" wrote: > > At Netdev 2.1 a meeting was held to discuss OvS offload. Minutes of the > discussion follow. I apologise in advance for any errors or omissions; > doubly for any errors in the attendee list. > > Topic: OVS Hardware Offload Using TC > Date: 7th April 2017 > Location: Netdev 2.1, Montreal > Attendees: Aaron Conole, Ben LaHaise, Eran Ben Elisha, Hannes Frederic Sowa, > Jakub Kicinski, Jiri Pirko, Joe Stringer, John Fastabend, Nick Viljoen, > Rashid Khan, Rony Efraim, Simon Horman > > Joe raised 2 concerns: > > 1) How to enable users to understand whether offload is >successful and if not, why not? > > a) There is functionality in the v7[1] patchset to report which flows > are present in hardware. > > b) New error reporting infrastructure from the kernel is forthcoming It > should allow TC to provide more error information if a flow can't be > added to hardware. This could be made available to users - e.g. logged > - to allow them better understand the reason for the failure. > > 2) Maintenance burden falling on existing maintainers > > a) Simon offered to take some of the maintenance burden > immediately as he is already a committer. > > b) The aim is to ensure that in future there are other committers > who are interested in this feature. > > There was consensus that if the feature-set does not grow there should be > discussion of deprecating the HW offload support provided by [1]. > > Joe raised issue of whether OVS should probe hardware capabilities at > runtime. > John suggested this may be complex; potential combinatorial set is too > large. > > Rony then raised the increased complexity of using multiple NICs of > different types with different offload capabilities, this was tabled to a > later date. > > Joe has expressed a desire for more testing. There was a general agreement > to contribute tests. > > [1] [PATCH ovs V7 00/24] Introducing HW offload support for openvswitch ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] Minutes: OvS Offload Discussion at Netdev 2.1
On Wed, Apr 12, 2017 at 03:01:29PM -0300, Flavio Leitner wrote: > On Sat, Apr 08, 2017 at 04:47:57PM -0400, Simon Horman wrote: > > At Netdev 2.1 a meeting was held to discuss OvS offload. Minutes of the > > discussion follow. I apologise in advance for any errors or omissions; > > doubly for any errors in the attendee list. > > > > Topic: OVS Hardware Offload Using TC > > Date: 7th April 2017 > > Location: Netdev 2.1, Montreal > > Attendees: Aaron Conole, Ben LaHaise, Eran Ben Elisha, Hannes Frederic Sowa, > > Jakub Kicinski, Jiri Pirko, Joe Stringer, John Fastabend, Nick Viljoen, > > Rashid Khan, Rony Efraim, Simon Horman > > > > Joe raised 2 concerns: > > > > 1) How to enable users to understand whether offload is > >successful and if not, why not? > > > > a) There is functionality in the v7[1] patchset to report which flows > > are present in hardware. > > > > b) New error reporting infrastructure from the kernel is forthcoming It > > should allow TC to provide more error information if a flow can't be > > added to hardware. This could be made available to users - e.g. logged > > - to allow them better understand the reason for the failure. > > > > 2) Maintenance burden falling on existing maintainers > > > > a) Simon offered to take some of the maintenance burden > > immediately as he is already a committer. > > > > b) The aim is to ensure that in future there are other committers > > who are interested in this feature. > > > > There was consensus that if the feature-set does not grow there should be > > discussion of deprecating the HW offload support provided by [1]. > > > > Joe raised issue of whether OVS should probe hardware capabilities at > > runtime. > > John suggested this may be complex; potential combinatorial set is too > > large. > > > > Rony then raised the increased complexity of using multiple NICs of > > different types with different offload capabilities, this was tabled to a > > later date. > > > > Joe has expressed a desire for more testing. There was a general agreement > > to contribute tests. > > > > [1] [PATCH ovs V7 00/24] Introducing HW offload support for openvswitch > > Thanks for the minutes. I wasn't there so this helps to understand > what has been discussed. > > Have you discussed how we are going to tag/document this feature? For > instance, are we going to say this is "experimental"? My recollection is that important detail was not discussed at the meeting. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows
On Tue, Apr 18, 2017 at 03:18:55PM +0300, Roi Dayan wrote:
>
>
> On 14/04/2017 04:11, Joe Stringer wrote:
> >On 7 April 2017 at 06:12, Roi Dayan wrote:
> >>From: Paul Blakey
> >>
> >>Signed-off-by: Paul Blakey
> >>Reviewed-by: Roi Dayan
> >>Reviewed-by: Simon Horman
> >>---
> >
> >
> >
> >>diff --git a/lib/netdev.h b/lib/netdev.h
> >>index d6c07c1..6d2db7d 100644
> >>--- a/lib/netdev.h
> >>+++ b/lib/netdev.h
> >>@@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct
> >>dp_packet_batch *,
> >> bool may_steal, bool concurrent_txq);
> >> void netdev_send_wait(struct netdev *, int qid);
> >>
> >>+/* Flow offloading. */
> >>+struct offload_info {
> >>+const void *port_hmap_obj; /* To query ports info from netdev port map
> >>*/
> >>+ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */
> >
> >Is this assuming there is only ever one tunnel destination port? What
> >about multiple output?
> >
>
> Yes. we currently only support single dst port output. if there is more than
> 1 we fail with EOPNOTSUPP and fallback to OVS datapath.
> the check is done in dpif-netlink.c parse_flow_put()
FWIW, from my PoV this is a shortcoming of the feature-set provided by
this patch-set and is something I would like to see lifted at some point in
the future - I would be interested in doing so if it isn't on anyone else's
todo list. Perhaps it would be useful to formally list what offloads are
supported by the API?
The above notwithstanding, I would expect that output to a single tunnel
vport covers a lot of use-cases.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] datapath: Fixups for MPLS GSO
Hi Yi-Hung,
thanks for taking on this difficult piece of work and apologies for the
delay in responding.
On Mon, Apr 03, 2017 at 04:24:36PM -0700, Yi-Hung Wei wrote:
> This commit backports the following upstream commits to fix MPLS GSO in ovs
> datapath. It has been tested on kernel 4.4 and 4.9.
Thanks also for noting the versions this has been tested against. I expect
there testing against other versions will show some residual problems but
I think that fixing 4.4 and 4.9 is a good step forwards.
I see that this patch backports 4 upstream patches. I am curious to know
if you considered backporting them individually. That would have made
reviewing a little easier for me.
> Upstream commit:
> commit 48d2ab609b6bbecb7698487c8579bc40de9d6dfa
> Author: David Ahern
> Date: Wed Aug 24 20:10:44 2016 -0700
...
> Upstream commit:
> commit f7d49bce8e741e1e6aa14ce4db1b6cea7e4be4e8
> Author: Jiri Benc
> Date: Fri Sep 30 19:08:05 2016 +0200
...
> Upstream commit:
> commit 85de4a2101acb85c3b1dde465e84596ccca99f2c
> Author: Jiri Benc
> Date: Fri Sep 30 19:08:07 2016 +0200
>
> openvswitch: use mpls_hdr
>
> skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
> instead.
>
> Signed-off-by: Jiri Benc
> Acked-by: Pravin B Shelar
> Signed-off-by: David S. Miller
...
> Upstream commit:
> commit c66549ffd05831abf6cf19ce0571ad868e39
> Author: Jiri Benc
> Date: Wed Oct 5 15:01:57 2016 +0200
...
> diff --git a/acinclude.m4 b/acinclude.m4
> index 744d8f89525c..82ca4a28015c 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -479,6 +479,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> [OVS_GREP_IFELSE([$KSRC/include/net/dst_cache.h],
> [dst_cache],
>
> [OVS_DEFINE([USE_UPSTREAM_TUNNEL])])])])
>
> + OVS_GREP_IFELSE([$KSRC/include/linux/mpls.h], [mpls_hdr])
Should the path be $KSRC/include/net/mpls.h?
I am looking at 9095e10edd28 ("mpls: move mpls_hdr to a common location")
>OVS_GREP_IFELSE([$KSRC/include/linux/net.h], [sock_create_kern.*net],
>[OVS_DEFINE([HAVE_SOCK_CREATE_KERN_NET])])
>OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h], [ndo_fill_metadata_dst])
> diff --git a/datapath/actions.c b/datapath/actions.c
> index 06080b24ea5a..ecc5136a3529 100644
> --- a/datapath/actions.c
> +++ b/datapath/actions.c
...
> @@ -169,20 +170,26 @@ static int push_mpls(struct sk_buff *skb, struct
> sw_flow_key *key,
> if (skb_cow_head(skb, MPLS_HLEN) < 0)
> return -ENOMEM;
>
> + if (!ovs_skb_get_inner_protocol(skb)) {
> + skb_set_inner_network_header(skb, skb->mac_len);
> + ovs_skb_set_inner_protocol(skb, skb->protocol);
> + }
> +
> skb_push(skb, MPLS_HLEN);
> memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
> skb->mac_len);
> skb_reset_mac_header(skb);
> +#ifdef HAVE_MPLS_HDR
> + skb_set_network_header(skb, skb->mac_len);
> +#endif
It is not clear to me why this call to skb_set_network_header() is
guarded by HAVE_MPLS_HDR here and moreover not elsewhere in this patch.
...
> @@ -198,21 +205,22 @@ static int pop_mpls(struct sk_buff *skb, struct
> sw_flow_key *key,
> if (unlikely(err))
> return err;
>
> - skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
> + skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
>
> memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
> skb->mac_len);
>
> __skb_pull(skb, MPLS_HLEN);
> skb_reset_mac_header(skb);
> + skb_set_network_header(skb, skb->mac_len);
...
> @@ -707,6 +715,12 @@ static int ovs_vport_output(OVS_VPORT_OUTPUT_PARAMS)
> skb_postpush_rcsum(skb, skb->data, data->l2_len);
> skb_reset_mac_header(skb);
>
> + if (eth_p_mpls(skb->protocol)) {
> + skb->inner_network_header = skb->network_header;
> + skb_set_network_header(skb, data->network_offset);
> + skb_reset_mac_len(skb);
> + }
> +
...
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2bc1ad0d5304..1d40b2400406 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
...
> @@ -679,12 +674,12 @@ static int key_extract(struct sk_buff *skb, struct
> sw_flow_key *key)
> if (unlikely(error))
> return 0;
>
> - memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
> + memcpy(&lse, skb_inner_network_header(skb), MPLS_HLEN);
>
> if (stack_len == MPLS_HLEN)
> memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
>
> - skb_set_network_header(skb, skb->mac_len + stack_len);
> + skb_set_inner_network_header(skb, skb->mac_len +
> stack_len);
> if (lse & htonl(MPLS_LS_S_MASK))
>
Re: [ovs-dev] [PATCH 2/3] datapath: Fix ovs_flow_key_update()
On Mon, Apr 03, 2017 at 04:24:37PM -0700, Yi-Hung Wei wrote:
> Upstream commit:
Now that this is upstream I think it would be useful to include the
following:
commit 6f56f6186c18e3fd54122b73da68e870687b8c59
Author: Yi-Hung Wei
Date: Thu Mar 30 12:36:03 2017 -0700
> ovs_flow_key_update() is called when the flow key is invalid, and it is
> used to update and revalidate the flow key. Commit 329f45bc4f19
> ("openvswitch: add mac_proto field to the flow key") introduces mac_proto
> field to flow key and use it to determine whether the flow key is valid.
> However, the commit does not update the code path in ovs_flow_key_update()
> to revalidate the flow key which may cause BUG_ON() on execute_recirc().
> This patch addresses the aforementioned issue.
>
> Fixes: 329f45bc4f19 ("openvswitch: add mac_proto field to the flow key")
> Signed-off-by: Yi-Hung Wei
> Acked-by: Jiri Benc
> Signed-off-by: David S. Miller
>
> Signed-off-by: Yi-Hung Wei
Nonetheless:
Acked-by: Simon Horman
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/3] system-traffic: Add test for mpls actions
On Mon, Apr 03, 2017 at 04:24:38PM -0700, Yi-Hung Wei wrote:
> Add ping test to verify the behavior of mpls_push/pop actions. In this
> test, we use the resubmit action to trigger recirulation for making sure
> the flow key is revalidated after mpls_push/pop. This test depends on
> commit 5ba0c107c51e ("datapath: Fix ovs_flow_key_update()") to behave
> correctly.
>
> Signed-off-by: Yi-Hung Wei
Very nice to see a test for this.
Acked-by: Simon Horman
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] datapath: Fixups for MPLS GSO
On Tue, Apr 25, 2017 at 05:45:34PM -0700, Yi-Hung Wei wrote:
> Hi Simon,
>
> Thanks for your review. Please find my relies below.
> I will sent out v2 based on your review.
>
> On Tue, Apr 25, 2017 at 12:43 AM, Simon Horman
> wrote:
> > Hi Yi-Hung,
> >
> > thanks for taking on this difficult piece of work and apologies for the
> > delay in responding.
> >
> > On Mon, Apr 03, 2017 at 04:24:36PM -0700, Yi-Hung Wei wrote:
> >> This commit backports the following upstream commits to fix MPLS GSO in ovs
> >> datapath. It has been tested on kernel 4.4 and 4.9.
> >
> > Thanks also for noting the versions this has been tested against. I expect
> > there testing against other versions will show some residual problems but
> > I think that fixing 4.4 and 4.9 is a good step forwards.
> >
> > I see that this patch backports 4 upstream patches. I am curious to know
> > if you considered backporting them individually. That would have made
> > reviewing a little easier for me.
> Thanks for your suggestion. I pull two independent patches out of this
> patch in v2.
> Commit 48d2ab609b6b("net: mpls: Fixups for GSO") and 85de4a2101ac
> ("openvswitch: use mpls_hdr") are backported together since I am using the
> mpls_hdr() in the later commit to backport some logic in the first commit.
Thanks.
> >> Upstream commit:
> >> commit 48d2ab609b6bbecb7698487c8579bc40de9d6dfa
> >> Author: David Ahern
> >> Date: Wed Aug 24 20:10:44 2016 -0700
> >
> > ...
> >
> >> Upstream commit:
> >> commit f7d49bce8e741e1e6aa14ce4db1b6cea7e4be4e8
> >> Author: Jiri Benc
> >> Date: Fri Sep 30 19:08:05 2016 +0200
> >
> > ...
> >
> >> Upstream commit:
> >> commit 85de4a2101acb85c3b1dde465e84596ccca99f2c
> >> Author: Jiri Benc
> >> Date: Fri Sep 30 19:08:07 2016 +0200
> >>
> >> openvswitch: use mpls_hdr
> >>
> >> skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
> >> instead.
> >>
> >> Signed-off-by: Jiri Benc
> >> Acked-by: Pravin B Shelar
> >> Signed-off-by: David S. Miller
> >
> > ...
> >
> >> Upstream commit:
> >> commit c66549ffd05831abf6cf19ce0571ad868e39
> >> Author: Jiri Benc
> >> Date: Wed Oct 5 15:01:57 2016 +0200
> >
> > ...
> >
> >> diff --git a/acinclude.m4 b/acinclude.m4
> >> index 744d8f89525c..82ca4a28015c 100644
> >> --- a/acinclude.m4
> >> +++ b/acinclude.m4
> >> @@ -479,6 +479,7 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >> [OVS_GREP_IFELSE([$KSRC/include/net/dst_cache.h],
> >> [dst_cache],
> >>
> >> [OVS_DEFINE([USE_UPSTREAM_TUNNEL])])])])
> >>
> >> + OVS_GREP_IFELSE([$KSRC/include/linux/mpls.h], [mpls_hdr])
> >
> > Should the path be $KSRC/include/net/mpls.h?
> >
> > I am looking at 9095e10edd28 ("mpls: move mpls_hdr to a common location")
> Yes, you're right. Thanks for finding this bug.
>
> >
> >>OVS_GREP_IFELSE([$KSRC/include/linux/net.h], [sock_create_kern.*net],
> >>[OVS_DEFINE([HAVE_SOCK_CREATE_KERN_NET])])
> >>OVS_GREP_IFELSE([$KSRC/include/linux/netdevice.h],
> >> [ndo_fill_metadata_dst])
> >> diff --git a/datapath/actions.c b/datapath/actions.c
> >> index 06080b24ea5a..ecc5136a3529 100644
> >> --- a/datapath/actions.c
> >> +++ b/datapath/actions.c
> >
> > ...
> >
> >> @@ -169,20 +170,26 @@ static int push_mpls(struct sk_buff *skb, struct
> >> sw_flow_key *key,
> >> if (skb_cow_head(skb, MPLS_HLEN) < 0)
> >> return -ENOMEM;
> >>
> >> + if (!ovs_skb_get_inner_protocol(skb)) {
> >> + skb_set_inner_network_header(skb, skb->mac_len);
> >> + ovs_skb_set_inner_protocol(skb, skb->protocol);
> >> + }
> >> +
> >> skb_push(skb, MPLS_HLEN);
> >> memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
> >> skb->mac_len);
> >> skb_reset_mac_header(skb);
> >> +#ifdef HAVE_MPLS_HDR
> >> + skb_set_network_header(skb, skb->mac_len);
> >> +#endif
> >
> > It is not clear to me why this call to skb_set_network_header() is
> > guarded by HAVE_MPLS_HDR here and moreover not else
Re: [ovs-dev] [PATCH v3 1/5] datapath: Fixups for MPLS GSO
Hi Yi-Hung, I'm having a little trouble following the status of this patchset. Would it be possible to repost v3 (or post v4 if that is pending) with: * A cover letter including: - An overview of the patchset - An overview of the changes between v2 and v3 (and v4 is there is one). Changes between v1 and v2 would also be nice but I think I have a handle on those. Alternatively the details of changes between revisions could be after changelog and a '---' in each patch * All (5) patches. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/5] Backport upstream mpls fix
On Mon, May 01, 2017 at 10:24:30AM -0700, Yi-Hung Wei wrote: > This series backports the upstream mpls fixes and add a system traffic > test for mpls actions. Thanks Yi-Hung, this looks good to me. Acked-by: Simon Horman Joe, were you considering applying this? I'm happy to do so myself if there are no objections. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V7 02/24] netdev: Adding a new netdev api to be used for offloading flows
On Wed, May 03, 2017 at 08:20:18AM +0300, Roi Dayan wrote:
>
>
> On 24/04/2017 14:35, Simon Horman wrote:
> >On Tue, Apr 18, 2017 at 03:18:55PM +0300, Roi Dayan wrote:
> >>
> >>
> >>On 14/04/2017 04:11, Joe Stringer wrote:
> >>>On 7 April 2017 at 06:12, Roi Dayan wrote:
> >>>>From: Paul Blakey
> >>>>
> >>>>Signed-off-by: Paul Blakey
> >>>>Reviewed-by: Roi Dayan
> >>>>Reviewed-by: Simon Horman
> >>>>---
> >>>
> >>>
> >>>
> >>>>diff --git a/lib/netdev.h b/lib/netdev.h
> >>>>index d6c07c1..6d2db7d 100644
> >>>>--- a/lib/netdev.h
> >>>>+++ b/lib/netdev.h
> >>>>@@ -156,6 +156,29 @@ int netdev_send(struct netdev *, int qid, struct
> >>>>dp_packet_batch *,
> >>>>bool may_steal, bool concurrent_txq);
> >>>>void netdev_send_wait(struct netdev *, int qid);
> >>>>
> >>>>+/* Flow offloading. */
> >>>>+struct offload_info {
> >>>>+const void *port_hmap_obj; /* To query ports info from netdev port
> >>>>map */
> >>>>+ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */
> >>>
> >>>Is this assuming there is only ever one tunnel destination port? What
> >>>about multiple output?
> >>>
> >>
> >>Yes. we currently only support single dst port output. if there is more than
> >>1 we fail with EOPNOTSUPP and fallback to OVS datapath.
> >>the check is done in dpif-netlink.c parse_flow_put()
> >
> >FWIW, from my PoV this is a shortcoming of the feature-set provided by
> >this patch-set and is something I would like to see lifted at some point in
> >the future - I would be interested in doing so if it isn't on anyone else's
> >todo list. Perhaps it would be useful to formally list what offloads are
> >supported by the API?
> >
> >The above notwithstanding, I would expect that output to a single tunnel
> >vport covers a lot of use-cases.
> >
>
> Yes, we do want to support multiple destinations and considered it as a
> later feature which is on our TODO, though we haven't started working on
> this yet.
> As for a formal list of what is offloads are supported, I guess we can list
> it later in the documentation.
That would be fine by me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/5] Backport upstream mpls fix
On Tue, May 02, 2017 at 10:38:17AM -0700, Joe Stringer wrote: > On 2 May 2017 at 05:03, Simon Horman wrote: > > On Mon, May 01, 2017 at 10:24:30AM -0700, Yi-Hung Wei wrote: > >> This series backports the upstream mpls fixes and add a system traffic > >> test for mpls actions. > > > > Thanks Yi-Hung, this looks good to me. > > > > Acked-by: Simon Horman > > > > Joe, were you considering applying this? > > I'm happy to do so myself if there are no objections. > > Thanks for reviewing these, Simon. I also took a look and they seem > fine to me. Please do apply them, it'll be good to get this resolved. Thanks, I have pushed these to master. Should the also be considered for branch-2.6, branch-2.5, etc...? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] tests: Only run python SSL test if SSL support is configured
Only run python SSL test, which invokes ovsdb with a --remote=pssl,
if SSL support is configured.
Without this change the following error appears when running
the test-suite when OVS is configured with --disable-ssl.
+ovsdb-server: Private key specified but Open vSwitch was built without SSL
support
./ovsdb-idl.at:1215: exit code was 1, expected 0
Fixes: d90ed7d65ba8 ("python: Add SSL support to the python ovs client library")
Cc: Numan Siddique
Signed-off-by: Simon Horman
---
tests/ovsdb-idl.at | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index d2c1ea6f3aaa..d28dfc11e786 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1183,6 +1183,7 @@ m4_define([OVSDB_CHECK_IDL_NOTIFY_PY],
# This test uses the Python IDL implementation with ssl
m4_define([OVSDB_CHECK_IDL_NOTIFY_SSL_PY],
[AT_SETUP([$1 - SSL])
+ AT_SKIP_IF([test "$HAVE_OPENSSL" = no])
AT_SKIP_IF([test $HAVE_PYTHON = no])
$PYTHON -m OpenSSL.SSL
SSL_PRESENT=$?
--
2.1.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tests: Only run python SSL test if SSL support is configured
On Wed, May 03, 2017 at 10:56:39AM -0700, Ben Pfaff wrote:
> On Wed, May 03, 2017 at 05:55:51PM +0200, Simon Horman wrote:
> > Only run python SSL test, which invokes ovsdb with a --remote=pssl,
> > if SSL support is configured.
> >
> > Without this change the following error appears when running
> > the test-suite when OVS is configured with --disable-ssl.
> >
> > +ovsdb-server: Private key specified but Open vSwitch was built without SSL
> > support
> > ./ovsdb-idl.at:1215: exit code was 1, expected 0
> >
> > Fixes: d90ed7d65ba8 ("python: Add SSL support to the python ovs client
> > library")
> > Cc: Numan Siddique
> > Signed-off-by: Simon Horman
>
> Good catch.
>
> Acked-by: Ben Pfaff
Thanks, applied.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 01/26] tc: Add tc flower interface
On Wed, May 03, 2017 at 06:07:52PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> Add tc flower interface that will be used to offload flows via tc
> flower classifier. Depending on the flag used (skip_sw/hw) flower
> will pass those to HW or handle them itself.
> Move some tc related functions from netdev-linux.c to tc.c
>
> Co-authored-by: Shahar Klein
> Signed-off-by: Shahar Klein
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> Reviewed-by: Simon Horman
> ---
> lib/automake.mk|2 +
> lib/netdev-linux.c | 164 ++--
> lib/tc.c | 1109
>
> lib/tc.h | 128 ++
> 4 files changed, 1279 insertions(+), 124 deletions(-)
> create mode 100644 lib/tc.c
> create mode 100644 lib/tc.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index faace79..3d57610 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -352,6 +352,8 @@ if LINUX
> lib_libopenvswitch_la_SOURCES += \
> lib/dpif-netlink.c \
> lib/dpif-netlink.h \
> + lib/tc.h \
> + lib/tc.c \
tc.c seems to contain two types of functions:
1. Code which is used by both (old) netdev-linux.c paths and
code which is used by (new) tc-flower specific paths.
For example tc_transact().
2. Code which is specific to tc-flower
The latter does not compile against old kernel headers.
As per Flavio Leitner's review or v7 it seems that the compilation problem
may be addressed by patch 23.
I think it would also be worth considering splitting the TC code such that
tc-flower specific code to is present in tc_flower.[ch] and leave shared
code is in tc.[ch].
Moving code to tc.[ch] could be a separate patch to adding tc_flower.[ch].
In my opinion smaller patches are easier to review and possibly merge
incrementally.
Overall this patch-set seems very large and I think it would be worth
considering ways to merge it incrementally.
...
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 79e8273..a6bb515 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
...
> @@ -2094,7 +2095,7 @@ netdev_linux_set_policing(struct netdev *netdev_,
>
> COVERAGE_INC(netdev_set_policing);
> /* Remove any existing ingress qdisc. */
> -error = tc_add_del_ingress_qdisc(netdev_, false);
> +error = tc_add_del_ingress_qdisc(ifindex, false);
This patch both changes the signature of tc_add_del_ingress_qdisc() and
moves it to tc.c. The signature change could be in a separate patch.
...
> @@ -2930,8 +2931,8 @@ codel_setup_qdisc__(struct netdev *netdev, uint32_t
> target, uint32_t limit,
>
> tc_del_qdisc(netdev);
>
> -tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
> -NLM_F_EXCL | NLM_F_CREATE, &request);
> +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
> + NLM_F_EXCL | NLM_F_CREATE,
> &request);
Likewise, I think reworking tc_make_request() could be a separate patch.
...
> @@ -4222,13 +4224,11 @@ hfsc_setup_qdisc__(struct netdev * netdev)
>
> tc_del_qdisc(netdev);
>
> -tcmsg = tc_make_request(netdev, RTM_NEWQDISC,
> -NLM_F_EXCL | NLM_F_CREATE, &request);
> -
> +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWQDISC,
> + NLM_F_EXCL | NLM_F_CREATE,
> &request);
> if (!tcmsg) {
> return ENODEV;
> }
> -
The change above seems spurious.
> tcmsg->tcm_handle = tc_make_handle(1, 0);
> tcmsg->tcm_parent = TC_H_ROOT;
>
> @@ -4255,12 +4255,11 @@ hfsc_setup_class__(struct netdev *netdev, unsigned
> int handle,
> struct ofpbuf request;
> struct tc_service_curve min, max;
>
> -tcmsg = tc_make_request(netdev, RTM_NEWTCLASS, NLM_F_CREATE, &request);
> -
> +tcmsg = netdev_linux_tc_make_request(netdev, RTM_NEWTCLASS,
> + NLM_F_CREATE, &request);
> if (!tcmsg) {
> return ENODEV;
> }
> -
Ditto.
> tcmsg->tcm_handle = handle;
> tcmsg->tcm_parent = parent;
>
...
> diff --git a/lib/tc.c b/lib/tc.c
> new file mode 100644
> index 000..cd06025
> --- /dev/null
> +++ b/lib/tc.c
> @@ -0,0 +1,1109 @@
...
> +static const struct nl_policy tca_flower_policy[] = {
> +[TCA_FLOWER_CLASSID] = { .type = NL_A_U32, .optional = true, },
> +[TCA_FLOWER_INDEV] = { .type = NL_A_STRING, .max_len = IFNAMSIZ,
> + .optional = true, },
> +[TCA_FLOWER_KEY_ETH_SRC] = { .type = NL_A_UNSPEC,
> + .min_len = ETH_ALEN, .optional = true, },
> +[TCA_FLOWER_KE
Re: [ovs-dev] [PATCH ovs V8 01/26] tc: Add tc flower interface
On Sun, May 07, 2017 at 02:46:14PM +0300, Roi Dayan wrote: > > > On 04/05/2017 19:35, Simon Horman wrote: > >On Wed, May 03, 2017 at 06:07:52PM +0300, Roi Dayan wrote: > >>From: Paul Blakey > >> > >>Add tc flower interface that will be used to offload flows via tc > >>flower classifier. Depending on the flag used (skip_sw/hw) flower > >>will pass those to HW or handle them itself. > >>Move some tc related functions from netdev-linux.c to tc.c > >> > >>Co-authored-by: Shahar Klein > >>Signed-off-by: Shahar Klein > >>Signed-off-by: Paul Blakey > >>Reviewed-by: Roi Dayan > >>Reviewed-by: Simon Horman > >>--- > >> lib/automake.mk|2 + > >> lib/netdev-linux.c | 164 ++-- > >> lib/tc.c | 1109 > >> > >> lib/tc.h | 128 ++ > >> 4 files changed, 1279 insertions(+), 124 deletions(-) > >> create mode 100644 lib/tc.c > >> create mode 100644 lib/tc.h > >> > >>diff --git a/lib/automake.mk b/lib/automake.mk > >>index faace79..3d57610 100644 > >>--- a/lib/automake.mk > >>+++ b/lib/automake.mk > >>@@ -352,6 +352,8 @@ if LINUX > >> lib_libopenvswitch_la_SOURCES += \ > >>lib/dpif-netlink.c \ > >>lib/dpif-netlink.h \ > >>+ lib/tc.h \ > >>+ lib/tc.c \ > > > >tc.c seems to contain two types of functions: > > > >1. Code which is used by both (old) netdev-linux.c paths and > > code which is used by (new) tc-flower specific paths. > > For example tc_transact(). > >2. Code which is specific to tc-flower > > > >The latter does not compile against old kernel headers. > > > >As per Flavio Leitner's review or v7 it seems that the compilation problem > >may be addressed by patch 23. > > this is correct. we did first all work for hw offload and then added a > compat fix commit. > Isn't it ok since there is no point for half work for hw offload? Its not ok because this patch does not compile which breaks bisection. It may be that Flavio's suggestion is not the best way to resolve the problem - another idea I have is to conditionally compile the tc_flower.c that I suggest below and provide stub functions in tc_flower.h for the case where tc_flower.c is not compiled. > >I think it would also be worth considering splitting the TC code such that > >tc-flower specific code to is present in tc_flower.[ch] and leave shared > >code is in tc.[ch]. > > > >Moving code to tc.[ch] could be a separate patch to adding tc_flower.[ch]. > >In my opinion smaller patches are easier to review and possibly merge > >incrementally. > > I agree that first commit should do only the moving and second to add new > code but most of the functions are flower related. I'm not sure how much > will stay in tc.c after removing flower related code to a new file. Thanks, I think that would make the patches rather a lot easier on the eyes. ... Thanks for your responses to the other, more specific, review comments. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 02/26] netdev: Adding a new netdev api to be used for offloading flows
On Wed, May 03, 2017 at 06:07:53PM +0300, Roi Dayan wrote:
> From: Paul Blakey
Please add some text to the changelog.
>
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> Reviewed-by: Simon Horman
...
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> new file mode 100644
> index 000..eb5e79a
> --- /dev/null
> +++ b/lib/netdev-tc-offloads.c
> @@ -0,0 +1,154 @@
> +/*
> + * Copyright (c) 2016 Mellanox Technologies, Ltd.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include
> +
> +#include "netdev-tc-offloads.h"
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "coverage.h"
> +#include "dp-packet.h"
> +#include "dpif-netlink.h"
> +#include "dpif-netdev.h"
> +#include "openvswitch/dynamic-string.h"
> +#include "fatal-signal.h"
> +#include "hash.h"
> +#include "openvswitch/hmap.h"
> +#include "netdev-provider.h"
> +#include "netdev-vport.h"
> +#include "netlink-notifier.h"
> +#include "netlink-socket.h"
> +#include "netlink.h"
> +#include "openvswitch/ofpbuf.h"
> +#include "openflow/openflow.h"
> +#include "ovs-atomic.h"
> +#include "packets.h"
> +#include "poll-loop.h"
> +#include "rtnetlink.h"
> +#include "openvswitch/shash.h"
> +#include "netdev-provider.h"
> +#include "openvswitch/match.h"
> +#include "openvswitch/vlog.h"
> +#include "tc.h"
Are all of the above headers needed for what follows?
There seems to be a lot of #includes above.
> +VLOG_DEFINE_THIS_MODULE(netdev_tc_offloads);
> +
> +int
> +netdev_tc_flow_flush(struct netdev *netdev OVS_UNUSED)
> +{
> +return EOPNOTSUPP;
> +}
> +
> +int
> +netdev_tc_flow_dump_create(struct netdev *netdev,
> + struct netdev_flow_dump **dump_out)
> +{
> +struct netdev_flow_dump *dump = xzalloc(sizeof *dump);
> +
> +dump->netdev = netdev_ref(netdev);
> +
> +*dump_out = dump;
> +
> +return 0;
> +}
> +
> +int
> +netdev_tc_flow_dump_destroy(struct netdev_flow_dump *dump)
> +{
> +netdev_close(dump->netdev);
> +free(dump);
> +
> +return 0;
> +}
> +
> +bool
> +netdev_tc_flow_dump_next(struct netdev_flow_dump *dump OVS_UNUSED,
> + struct match *match OVS_UNUSED,
> + struct nlattr **actions OVS_UNUSED,
> + struct dpif_flow_stats *stats OVS_UNUSED,
> + ovs_u128 *ufid OVS_UNUSED,
> + struct ofpbuf *rbuffer OVS_UNUSED,
> + struct ofpbuf *wbuffer OVS_UNUSED)
> +{
> +return false;
> +}
> +
> +int
> +netdev_tc_flow_put(struct netdev *netdev OVS_UNUSED,
> + struct match *match OVS_UNUSED,
> + struct nlattr *actions OVS_UNUSED,
> + size_t actions_len OVS_UNUSED,
> + struct dpif_flow_stats *stats OVS_UNUSED,
> + const ovs_u128 *ufid OVS_UNUSED,
Here and above ufid follows stats.
> + struct offload_info *info OVS_UNUSED)
> +{
> +return EOPNOTSUPP;
> +}
> +
> +int
> +netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
> + struct match *match OVS_UNUSED,
> + struct nlattr **actions OVS_UNUSED,
> + const ovs_u128 *ufid OVS_UNUSED,
> + struct dpif_flow_stats *stats OVS_UNUSED,
But here the order is reversed.
I always think that when a reviewer asks for entries to be sorted
Re: [ovs-dev] [PATCH ovs V8 03/26] other-config: Add hw-offload switch to control netdev flow offloading
On Wed, May 03, 2017 at 06:07:54PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Add a new configuration option - hw-offload that enables netdev > flow api. Enabling this option will allow offloading flows > using netdev implementation instead of the kernel datapath. > This configuration option defaults to false - disabled. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman ... This does not appear to address Flavio Leitner's review of v7 of this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 04/26] other-config: Add tc-policy switch to control tc flower flag
On Wed, May 03, 2017 at 06:07:55PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Add a new configuration tc-policy option that controls tc > flower flag. Possible options are none, skip_sw, skip_hw. > The default is none which is to insert the rule both to sw and hw. > This option is only relevant if hw-offload is enabled. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman This does not appear to address Flavio Leitner's review of v7 of this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 05/26] dpif: Save added ports in a port map for netdev flow api use
On Wed, May 03, 2017 at 06:07:56PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> To use netdev flow offloading api, dpifs needs to iterate over
> added ports. This addition inserts the added dpif ports in a hash map,
> The map will also be used to translate dpif ports to netdevs.
>
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> Reviewed-by: Simon Horman
...
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 7435fdf..9aa7e5e 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
> extern bool netdev_flow_api_enabled;
> void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
>
> +struct dpif_port;
> +int netdev_ports_insert(struct netdev *, const void *obj, struct dpif_port
> *);
> +struct netdev *netdev_ports_get(odp_port_t port, const void *obj);
> +int netdev_ports_remove(odp_port_t port, const void *obj);
> +odp_port_t netdev_ifindex_to_odp_port(int ifindex);
> +
> /* native tunnel APIs */
> /* Structure to pass parameters required to build a tunnel header. */
> struct netdev_tnl_build_header_params {
This patch seems to only partially address the review provided
by Joe Stringer for v7. In particular:
* netdev_ports_get() -> netdev_ports_lookup()
* Feedback regarding 'obj' being a not particularly clear abstraction.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 06/26] dpif-netlink: Flush added ports using netdev flow api
On Wed, May 03, 2017 at 06:07:57PM +0300, Roi Dayan wrote: > From: Paul Blakey > > If netdev flow offloading is enabled, flush all > added ports using netdev flow api. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman Flavio Leitner provided an Ack for v7 of this patch. You may want to consider adding that tag. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 07/26] netdev-tc-offloads: Implement netdev flow flush using tc interface
On Wed, May 03, 2017 at 06:07:58PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman This does not appear to address Flavio Leitner's review of v7 of this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 13/26] netdev-tc-offloads: Implement netdev flow put using tc interface
On Wed, May 03, 2017 at 06:08:04PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> Currently only tunnel offload is supported.
>
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> Reviewed-by: Simon Horman
> ---
> lib/dpif-netlink.c | 4 +-
> lib/netdev-tc-offloads.c | 392
> ++-
> 2 files changed, 385 insertions(+), 11 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 81d513d..7e6c647 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1937,8 +1937,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
> dpif_flow_put *put)
> return EOPNOTSUPP;
> }
>
> -memset(&match, 0, sizeof match);
> -
> err = parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
>put->mask_len, &match);
> if (err) {
> @@ -2011,7 +2009,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct
> dpif_flow_put *put)
>
> VLOG_DBG("added flow");
> } else if (err != EEXIST) {
> -VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
> +VLOG_ERR_RL(&rl, "failed adding flow: %s", ovs_strerror(err));
> }
>
> out:
I may be misunderstanding things but perhaps the above changes
belong in an earlier patch?
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 7e33fff..e3daf62 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -461,16 +461,392 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
> return false;
> }
...
> +static int
> +test_key_and_mask(struct match *match) {
> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +const struct flow *key = &match->flow;
> +struct flow *mask = &match->wc.masks;
> +
> +if (mask->pkt_mark) {
> +VLOG_DBG_RL(&rl, "offloading attribute pkt_mark isn't supported");
> +return EOPNOTSUPP;
> +}
> +
> +if (mask->recirc_id && key->recirc_id != 0) {
> +VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported");
> +return EOPNOTSUPP;
> +}
> +mask->recirc_id = 0;
Its not clear to me why the recirc_id mask is reset here.
> +
> +if (mask->dp_hash) {
> +VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported");
> +return EOPNOTSUPP;
> +}
...
> +if (mask->metadata != 0) {
It seems to me that "if (!mask->metdata)" would make the above more
consistent with other checks in this function.
> +VLOG_DBG_RL(&rl, "offloading attribute metadata isn't supported");
> +return EOPNOTSUPP;
> +}
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 21/26] dpctl: Add an option to dump only certain kinds of flows
On Wed, May 03, 2017 at 06:08:12PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Usage: > # to dump all datapath flows (default): > ovs-dpctl dump-flows > > # to dump only flows that in kernel datapath: > ovs-dpctl dump-flows type=ovs > > # to dump only flows that are offloaded: > ovs-dpctl dump-flows type=offloaded > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman As per Flavio's review of v7, can this be made to return an error if type is invalid. I may have missed something I'm not seeing that in this patch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 21/26] dpctl: Add an option to dump only certain kinds of flows
On Wed, May 03, 2017 at 06:08:12PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> Usage:
> # to dump all datapath flows (default):
> ovs-dpctl dump-flows
>
> # to dump only flows that in kernel datapath:
> ovs-dpctl dump-flows type=ovs
>
> # to dump only flows that are offloaded:
> ovs-dpctl dump-flows type=offloaded
...
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 11be857..f534de2 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
...
> @@ -774,22 +775,29 @@ dpctl_dump_flows(int argc, const char *argv[], struct
> dpctl_params *dpctl_p)
> struct dpif_flow_dump *flow_dump;
> struct dpif_flow f;
> int pmd_id = PMD_ID_NULL;
> +int lastargc = 0;
> int error;
>
> -if (argc > 1 && !strncmp(argv[argc - 1], "filter=", 7)) {
> -filter = xstrdup(argv[--argc] + 7);
> +while (argc > 1 && lastargc != argc) {
> +lastargc = argc;
> +if (!strncmp(argv[argc - 1], "filter=", 7)) {
> +filter = xstrdup(argv[--argc] + 7);
> +} else if (!strncmp(argv[argc - 1], "type=", 5)) {
> +type = xstrdup(argv[--argc] + 5);
> +}
The above seems to have the presumably unintended side-effect of parsing
multiple filter= and type= arguments.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 25/26] dpif: Refactor flow logging functions to be used by other modules
On Wed, May 03, 2017 at 06:08:16PM +0300, Roi Dayan wrote: > Signed-off-by: Roi Dayan > Reviewed-by: Paul Blakey Reviewed-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 26/26] dpif-netlink: Use dpif logging functions
On Wed, May 03, 2017 at 06:08:17PM +0300, Roi Dayan wrote:
Please add some changelog text here.
> Signed-off-by: Roi Dayan
> Reviewed-by: Paul Blakey
> ---
> lib/dpif-netlink.c | 55
> +-
> 1 file changed, 17 insertions(+), 38 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 048dae6..3638358 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2113,35 +2113,11 @@ out:
> return err;
> }
>
> -static void
> -dbg_print_flow(const struct nlattr *key, size_t key_len,
> - const struct nlattr *mask, size_t mask_len,
> - const struct nlattr *actions, size_t actions_len,
> - const ovs_u128 *ufid,
> - const char *op)
> -{
> -struct ds s;
> -
> -ds_init(&s);
> -ds_put_cstr(&s, op);
> -ds_put_cstr(&s, " (");
> -odp_format_ufid(ufid, &s);
> -ds_put_cstr(&s, ")");
> -if (key_len) {
> -ds_put_cstr(&s, "\nflow (verbose): ");
> -odp_flow_format(key, key_len, mask, mask_len, NULL, &s, true);
> -ds_put_cstr(&s, "\nflow: ");
> -odp_flow_format(key, key_len, mask, mask_len, NULL, &s, false);
> -}
> -ds_put_cstr(&s, "\nactions: ");
> -format_odp_actions(&s, actions, actions_len);
> -VLOG_DBG("\n%s", ds_cstr(&s));
> -ds_destroy(&s);
> -}
> -
> static int
> try_send_to_netdev(struct dpif_netlink *dpif, struct dpif_op *op)
> {
> +int err = EOPNOTSUPP;
> +
It is unclear to me how the re-arangement of err or handling
is related to the stated goal of using dpif logging functions.
> switch (op->type) {
> case DPIF_OP_FLOW_PUT: {
> struct dpif_flow_put *put = &op->u.flow_put;
> @@ -2149,10 +2125,10 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct
> dpif_op *op)
> if (!put->ufid) {
> break;
> }
> -dbg_print_flow(put->key, put->key_len, put->mask, put->mask_len,
> - put->actions, put->actions_len, put->ufid,
> - (put->flags & DPIF_FP_MODIFY ? "PUT(MODIFY)" :
> "PUT"));
> -return parse_flow_put(dpif, put);
> +
> +log_flow_put_message(&dpif->dpif, &this_module, put, 0);
> +err = parse_flow_put(dpif, put);
> +break;
> }
> case DPIF_OP_FLOW_DEL: {
> struct dpif_flow_del *del = &op->u.flow_del;
> @@ -2160,10 +2136,11 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct
> dpif_op *op)
> if (!del->ufid) {
> break;
> }
> -dbg_print_flow(del->key, del->key_len, NULL, 0, NULL, 0,
> - del->ufid, "DEL");
> -return netdev_ports_flow_del(DPIF_HMAP_KEY(&dpif->dpif), del->ufid,
> - del->stats);
> +
> +log_flow_del_message(&dpif->dpif, &this_module, del, 0);
> +err = netdev_ports_flow_del(DPIF_HMAP_KEY(&dpif->dpif), del->ufid,
> +del->stats);
> +break;
> }
> case DPIF_OP_FLOW_GET: {
> struct dpif_flow_get *get = &op->u.flow_get;
> @@ -2171,15 +2148,17 @@ try_send_to_netdev(struct dpif_netlink *dpif, struct
> dpif_op *op)
> if (!op->u.flow_get.ufid) {
> break;
> }
> -dbg_print_flow(get->key, get->key_len, NULL, 0, NULL, 0,
> - get->ufid, "GET");
> -return parse_flow_get(dpif, get);
> +
> +log_flow_get_message(&dpif->dpif, &this_module, get, 0);
> +err = parse_flow_get(dpif, get);
> +break;
> }
> case DPIF_OP_EXECUTE:
> default:
> break;
> }
> -return EOPNOTSUPP;
> +
> +return err;
> }
>
> static void
> --
> 2.7.4
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 25/26] dpif: Refactor flow logging functions to be used by other modules
On Wed, May 10, 2017 at 05:19:26PM +0200, Simon Horman wrote: > On Wed, May 03, 2017 at 06:08:16PM +0300, Roi Dayan wrote: > > Signed-off-by: Roi Dayan > > Reviewed-by: Paul Blakey > > Reviewed-by: Simon Horman Sorry, I didn't notice before but could you add some text to the changelog? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovs V8 05/26] dpif: Save added ports in a port map for netdev flow api use
On Tue, May 16, 2017 at 12:16:46PM +0300, Roi Dayan wrote:
>
>
> On 08/05/2017 15:49, Simon Horman wrote:
> >On Wed, May 03, 2017 at 06:07:56PM +0300, Roi Dayan wrote:
> >>From: Paul Blakey
> >>
> >>To use netdev flow offloading api, dpifs needs to iterate over
> >>added ports. This addition inserts the added dpif ports in a hash map,
> >>The map will also be used to translate dpif ports to netdevs.
> >>
> >>Signed-off-by: Paul Blakey
> >>Reviewed-by: Roi Dayan
> >>Reviewed-by: Simon Horman
> >
> >...
> >
> >>diff --git a/lib/netdev.h b/lib/netdev.h
> >>index 7435fdf..9aa7e5e 100644
> >>--- a/lib/netdev.h
> >>+++ b/lib/netdev.h
> >>@@ -181,6 +181,12 @@ int netdev_init_flow_api(struct netdev *);
> >> extern bool netdev_flow_api_enabled;
> >> void netdev_set_flow_api_enabled(const struct smap *ovs_other_config);
> >>
> >>+struct dpif_port;
> >>+int netdev_ports_insert(struct netdev *, const void *obj, struct dpif_port
> >>*);
> >>+struct netdev *netdev_ports_get(odp_port_t port, const void *obj);
> >>+int netdev_ports_remove(odp_port_t port, const void *obj);
> >>+odp_port_t netdev_ifindex_to_odp_port(int ifindex);
> >>+
> >> /* native tunnel APIs */
> >> /* Structure to pass parameters required to build a tunnel header. */
> >> struct netdev_tnl_build_header_params {
> >
> >This patch seems to only partially address the review provided
> >by Joe Stringer for v7. In particular:
> >
> >* netdev_ports_get() -> netdev_ports_lookup()
> >* Feedback regarding 'obj' being a not particularly clear abstraction.
> >
>
> we did refactor all functions to have prefix netdev_ports_*
> there are both functions netdev_ports_get() and netdev_ports_lookup().
> did I miss something?
Thanks, I see that now. Looks good.
> about 'obj', I mentioned this in the changelog that it's left out for now
> and could be done in followup commit. is it ok?
Yes, that is fine by me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 01/31] netdev-linux: Refactor two tc functions
On Sun, May 28, 2017 at 02:59:43PM +0300, Roi Dayan wrote: > Refactor tc_make_request and tc_add_del_ingress_qdisc to accept > ifindex instead of netdev struct. > We later want to move those outside netdev-linux module to be > used by other modules. > This patch doesn't change any functionality. > > Signed-off-by: Paul Blakey > Signed-off-by: Roi Dayan Thanks, this looks good to me. I would be happy to apply it if someone provided a review. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 02/31] tc: Introduce tc module
On Sun, May 28, 2017 at 02:59:44PM +0300, Roi Dayan wrote:
> Add tc module to expose tc operations to be used by other modules.
> Move some tc related functions from netdev-linux.c to tc.c
> This patch doesn't change any functionality.
>
> Signed-off-by: Paul Blakey
> Signed-off-by: Roi Dayan
Thanks, this looks good to me modulo the minor suggestion below.
I would be happy to apply it if someone provided a review.
...
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 4fc3f6b..7a0517b 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
...
> @@ -4643,44 +4634,6 @@ static double ticks_per_s;
> */
> static unsigned int buffer_hz;
>
> -/* Returns tc handle 'major':'minor'. */
> -static unsigned int
> -tc_make_handle(unsigned int major, unsigned int minor)
> -{
> -return TC_H_MAKE(major << 16, minor);
> -}
> -
> -/* Returns the major number from 'handle'. */
> -static unsigned int
> -tc_get_major(unsigned int handle)
> -{
> -return TC_H_MAJ(handle) >> 16;
> -}
> -
> -/* Returns the minor number from 'handle'. */
> -static unsigned int
> -tc_get_minor(unsigned int handle)
> -{
> -return TC_H_MIN(handle);
> -}
...
> diff --git a/lib/tc.c b/lib/tc.c
> new file mode 100644
> index 000..644f30c
> --- /dev/null
> +++ b/lib/tc.c
> @@ -0,0 +1,114 @@
...
> +VLOG_DEFINE_THIS_MODULE(tc);
> +
> +/* Returns tc handle 'major':'minor'. */
> +unsigned int
> +tc_make_handle(unsigned int major, unsigned int minor)
> +{
> +return TC_H_MAKE(major << 16, minor);
> +}
> +
> +/* Returns the major number from 'handle'. */
> +unsigned int
> +tc_get_major(unsigned int handle)
> +{
> +return TC_H_MAJ(handle) >> 16;
> +}
> +
> +/* Returns the minor number from 'handle'. */
> +unsigned int
> +tc_get_minor(unsigned int handle)
> +{
> +return TC_H_MIN(handle);
> +}
I think that some consideration should be given to a follow-up patch to
move the above functions to tc.h and making them static inline to avoid
function-call overhead.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 03/31] compat: Add tc compatibility headers for old kernels
On Sun, May 28, 2017 at 02:59:45PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Added compatibility headers for actions vlan and tunnel key. > > Do not use compat code when compiling kernel datapath > there is no need for it as TC compatibility is not provided there. > In other words, the compat code is only used when compiling user-space > code against old kernel headers. > > Signed-off-by: Paul Blakey > Signed-off-by: Simon Horman > Reviewed-by: Roi Dayan > Acked-by: Flavio Leitner Thanks, applied to master branch. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 04/31] tc: Add tc flower functions
On Sun, May 28, 2017 at 02:59:46PM +0300, Roi Dayan wrote:
> Add tc helper functions to query and manipulate the flower classifier.
>
> Signed-off-by: Paul Blakey
> Signed-off-by: Roi Dayan
Thanks, this looks good to me and I would be happy to apply it if
someone provided a review.
Also, please see the note below.
...
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1,5 +1,6 @@
...
> +if (ip_proto == IPPROTO_TCP) {
> +if (attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]) {
> +key->src_port =
> +nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC]);
> +mask->src_port =
> +nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_SRC_MASK]);
> +}
> +if (attrs[TCA_FLOWER_KEY_TCP_DST_MASK]) {
> +key->dst_port =
> +nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST]);
> +mask->dst_port =
> +nl_attr_get_be16(attrs[TCA_FLOWER_KEY_TCP_DST_MASK]);
> +}
> +} else if (ip_proto == IPPROTO_UDP) {
> +if (attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]) {
> +key->src_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC]);
> +mask->src_port =
> +nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_SRC_MASK]);
> +}
> +if (attrs[TCA_FLOWER_KEY_UDP_DST_MASK]) {
> +key->dst_port = nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST]);
> +mask->dst_port =
> +nl_attr_get_be16(attrs[TCA_FLOWER_KEY_UDP_DST_MASK]);
> +}
> +}
As mentioned a few times it seems that SCTP could be trivially supported
here. I think adding it would make for an excellent follow-up patch.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 05/31] netdev: Adding a new netdev API to be used for offloading flows
On Sun, May 28, 2017 at 02:59:47PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> Add a new API interface for offloading dpif flows to netdev.
> The API consist on the following:
> flow_put - offload a new flow
> flow_get - query an offloaded flow
> flow_del - delete an offloaded flow
> flow_flush - flush all offloaded flows
> flow_dump_* - dump all offloaded flows
>
> In upcoming commits we will introduce an implementation of this
> API for netdev-linux.
>
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> Reviewed-by: Simon Horman
This looks good to me, modulo the minor nits below, and I would
be happy to apply it if someone provided a review.
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> new file mode 100644
> index 000..46017d8
> --- /dev/null
> +++ b/lib/netdev-tc-offloads.c
> @@ -0,0 +1,115 @@
...
> +int
> +netdev_tc_init_flow_api(struct netdev *netdev OVS_UNUSED)
> +{
> +return 0;
> +}
> +
This introduces a blank line at the end of a file.
...
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 39093e8..2cad5cb 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -847,7 +847,16 @@ get_stats(const struct netdev *netdev, struct
> netdev_stats *stats)
...
> +bool
> +netdev_flow_dump_next(struct netdev_flow_dump *dump,
> + struct match *match,
> + struct nlattr **actions,
> + struct dpif_flow_stats *stats,
> + ovs_u128 *ufid,
> + struct ofpbuf *rbuffer,
> + struct ofpbuf *wbuffer)
> +{
It looks some of the lines in the function declaration could be
consolidated.
> +const struct netdev_class *class = dump->netdev->netdev_class;
> +
> +return (class->flow_dump_next
> +? class->flow_dump_next(dump, match, actions, stats, ufid,
> +rbuffer, wbuffer)
> +: false);
> +}
> +
> +int
> +netdev_flow_put(struct netdev *netdev, struct match *match,
> +struct nlattr *actions, size_t act_len,
> +const ovs_u128 *ufid, struct offload_info *info,
> +struct dpif_flow_stats *stats)
> +{
Like these ones are.
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 07/31] other-config: Add tc-policy switch to control tc flower flag
On Sun, May 28, 2017 at 02:59:49PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> Add a new configuration tc-policy option that controls tc
> flower flag. Possible options are none, skip_sw, skip_hw.
> The default is none which is to insert the rule both to sw and hw.
> This option is only relevant if hw-offload is enabled.
>
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> Reviewed-by: Simon Horman
Thanks, the comment below not withstanding this looks good to me
and I would be happy to apply it if someone provided a review.
> ---
> lib/netdev.c | 6 ++
> lib/tc.c | 43 ++-
> lib/tc.h | 3 +++
> vswitchd/vswitch.xml | 17 +
> 4 files changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 2807a9d..19d809b 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -54,6 +54,9 @@
> #include "openvswitch/vlog.h"
> #include "flow.h"
> #include "util.h"
> +#ifdef __linux__
> +#include "tc.h"
> +#endif
>
> VLOG_DEFINE_THIS_MODULE(netdev);
>
> @@ -2117,6 +2120,9 @@ netdev_set_flow_api_enabled(const struct smap
> *ovs_other_config)
>
> VLOG_INFO("netdev: Flow API Enabled");
>
> +tc_set_policy(smap_get_def(ovs_other_config, "tc-policy",
> + TC_POLICY_DEFAULT));
> +
> ovsthread_once_done(&once);
> }
> }
In v7 Flavio Leitner made the following comment regarding this.
"Well, this enforces that the user sets tc-policy before enabling HW
offloading, otherwise it won't get set, right?"
My comment, along the same lines, is that it looks like it would be better
if tc_set_policy() was called independent of netdev_set_flow_api_enabled()
to allow them to be configured independently.
I realise that at this point the patch-set has some limitations regarding
restarting ovs when offloading of flows is enabled and disabled. And
probably the code above plays well with that restriction in place. But I
think it would be good to look into ways to lift that restriction and thus
it would seems worthwhile revisiting where tc_set_policy() is called.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 06/31] other-config: Add hw-offload switch to control netdev flow offloading
On Sun, May 28, 2017 at 02:59:48PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Add a new configuration option - hw-offload that enables netdev > flow api. Enabling this option will allow offloading flows > using netdev implementation instead of the kernel datapath. > This configuration option defaults to false - disabled. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman Thanks, this looks good to me. I would be happy to apply it if someone provided a review. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 09/31] dpif-netlink: Flush added ports using netdev flow api
On Sun, May 28, 2017 at 02:59:51PM +0300, Roi Dayan wrote: > From: Paul Blakey > > If netdev flow offloading is enabled, flush all > added ports using netdev flow api. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman > Acked-by: Flavio Leitner Thanks, this looks good to me. As it has been acked I would I would be happy to apply it once earlier in the patches in the series have been (reviewed and) applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 08/31] dpif: Save added ports in a port map for netdev flow api use
On Sun, May 28, 2017 at 02:59:50PM +0300, Roi Dayan wrote: > From: Paul Blakey > > To use netdev flow offloading api, dpifs needs to iterate over > added ports. This addition inserts the added dpif ports in a hash map, > The map will also be used to translate dpif ports to netdevs. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman > Acked-by: Flavio Leitner Thanks, this looks good to me. As it has been acked I would I would be happy to apply it once earlier in the patches in the series have been (reviewed and) applied. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V9 10/31] netdev-tc-offloads: Implement netdev flow flush using tc interface
On Sun, May 28, 2017 at 02:59:52PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan > Reviewed-by: Simon Horman > --- > lib/netdev-tc-offloads.c | 14 -- > 1 file changed, 12 insertions(+), 2 deletions(-) Thanks, this looks good to me. I would be happy to apply it if someone provided a review. I am taking a pause in looking over the patches now - I have looked over 1 - 10 and they seem good to me. I applied 03/31 which has been reviewed. And I will look out for reviews (by others) of those patches which have not been reviewed with a view to applying more patches. Please feel free to post v10 whenever you feel it is appropriate. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix false errors on interfaces without tunnel config
On Thu, Jul 27, 2017 at 02:40:02PM +0300, Roi Dayan wrote:
> When we skip adding a port using rtnetlink and not because of an error we
> need to return EOPNOTSUPP to avoid logging an error message.
>
> Fixes: 2fd3d5eda508 ("dpif-netlink-rtnl: Support layer3 GRE")
> Signed-off-by: Roi Dayan
> Reviewed-by: Paul Blakey
Thanks, this looks good to me. I would be happy to apply if someone
provided an Ack. I'd also be happy if someone else applied it with:
Acked-by: Simon Horman
> ---
> lib/dpif-netlink-rtnl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 98a2f2b..3efa1f6 100644
> --- a/lib/dpif-netlink-rtnl.c
> +++ b/lib/dpif-netlink-rtnl.c
> @@ -349,7 +349,7 @@ dpif_netlink_rtnl_port_create(struct netdev *netdev)
> type = netdev_to_ovs_vport_type(netdev_get_type(netdev));
> tnl_cfg = netdev_get_tunnel_config(netdev);
> if (!tnl_cfg) {
> -return EINVAL;
> +return EOPNOTSUPP;
> }
>
> kind = vport_type_to_kind(type, tnl_cfg);
> --
> 2.7.4
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] tc: Split IPs and transport layer ports unions in flower struct
On Thu, Jul 27, 2017 at 01:19:59PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Split dst/src_port and ipv4/ipv6 union so we can > distingush them easily for later features. The implications of this change on the size of struct tc_flower_key seem somewhat undesirable to me. Perhaps there is another way to distinguish between protocols? > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/3] tc: Refactor nl_msg_put_flower_options
On Thu, Jul 27, 2017 at 01:19:58PM +0300, Roi Dayan wrote: > From: Paul Blakey > > Refactor nl_msg_put_flower_options to be more readable. > This commit doesn't change functionality. > > Signed-off-by: Paul Blakey > Reviewed-by: Roi Dayan LGTM Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/3] netdev-tc-offloads: Parse ip related fields only if eth type is ip
On Thu, Jul 27, 2017 at 01:20:00PM +0300, Roi Dayan wrote: > From: Paul Blakey > > No need to parse ip related fields otherwise. I would prefer if the changelog could be read without reference to the subject. As you are planning to respin anyway perhaps you could fix this up. The code changes look fine to me. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tc: Correct convert ticks to msecs on parsing tc TM
On Thu, Jul 27, 2017 at 01:20:07PM +0300, Or Gerlitz wrote: > On 7/27/2017 1:14 PM, Roi Dayan wrote: > >the system call is done only once. > > good to know, would be worth to mention that on the change-log, so it's > clear we're good w.r.t performance. Roi, could you respin this patch with an updated changelog as requested by Or? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix false errors on interfaces without tunnel config
On Tue, Aug 01, 2017 at 04:48:02PM -0700, Joe Stringer wrote:
> On 1 August 2017 at 13:57, Joe Stringer wrote:
> > On 28 July 2017 at 05:43, Simon Horman wrote:
> >> On Thu, Jul 27, 2017 at 02:40:02PM +0300, Roi Dayan wrote:
> >>> When we skip adding a port using rtnetlink and not because of an error we
> >>> need to return EOPNOTSUPP to avoid logging an error message.
> >>>
> >>> Fixes: 2fd3d5eda508 ("dpif-netlink-rtnl: Support layer3 GRE")
> >>> Signed-off-by: Roi Dayan
> >>> Reviewed-by: Paul Blakey
> >>
> >> Thanks, this looks good to me. I would be happy to apply if someone
> >> provided an Ack. I'd also be happy if someone else applied it with:
> >>
> >> Acked-by: Simon Horman
> >
> > Acked-by: Joe Stringer
>
> I needed this to test Eric's recent VXLAN non-GPE fix, so I applied
> this to master.
Thanks
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] netdev-tc-offloads: Fix parsing SCTP in dump flows
On Sun, Aug 06, 2017 at 10:54:59AM +0300, Roi Dayan wrote:
> After splitting the unions of tcp/udp the sctp was forgotten
> when parsing flower back to match.
>
> Fixes: 2b1d9fa90909 ("tc: Split IPs and transport layer ports unions in
> flower struct")
> Signed-off-by: Roi Dayan
> Reviewed-by: Paul Blakey
Acked-by: Simon Horman
> ---
> lib/netdev-tc-offloads.c |3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index e51e638..2af1546 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -326,6 +326,9 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> } else if (key->ip_proto == IPPROTO_UDP) {
> match_set_tp_dst_masked(match, key->udp_dst, mask->udp_dst);
> match_set_tp_src_masked(match, key->udp_src, mask->udp_src);
> +} else if (key->ip_proto == IPPROTO_SCTP) {
> +match_set_tp_dst_masked(match, key->sctp_dst, mask->sctp_dst);
> +match_set_tp_src_masked(match, key->sctp_src, mask->sctp_src);
> }
> }
>
> --
> 1.7.1
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/7] Add offload support for ip ttl and tcp flags
On Mon, Aug 07, 2017 at 06:19:04PM +0300, Roi Dayan wrote: > Hi, > > This series adds support for offloading ip ttl and tcp flags > using tc interface. This looks nice, thanks. Acked-by: Simon Horman I'm also happy to apply this if someone else provides a review. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] netdev-vport: Always implement get_ifindex for netdev-vport
On Tue, Aug 08, 2017 at 07:36:25AM +0300, Roi Dayan wrote: > > > On 07/08/2017 20:05, Ben Pfaff wrote: > >On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote: > >>On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote: > >>>From: Paul Blakey > >>> > >>>Always implement get_ifindex without checking if offload is > >>>enabled or not as this should not be related. From ovs-dpctl > >>>we cannot tell if offload is enabled or not as other_config is > >>>not being read. > >>> > >>>Signed-off-by: Paul Blakey > >>>Reviewed-by: Roi Dayan > >> > >>Applied to master and branch-2.8, thanks! > > > >Sorry, I had to revert this because it caused several unit test > >failures: 770 781 783 787 788 791 2189 2378. > > > > This is because of the warnings from get_ifindex which resolved in > the second patch but was missing the ratelimiting you mentioned. > I submitted V2 of it to add back the ratelimiting > "netdev-linux: Reduce log level for ENODEV errors getting ifindex" In that case shouldn't the patch order be reversed to avoid the (temporary) regression Ben pointed out? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] netdev-vport: Always implement get_ifindex for netdev-vport
On Tue, Aug 08, 2017 at 01:35:41PM +0300, Roi Dayan wrote: > > > On 08/08/2017 11:04, Simon Horman wrote: > >On Tue, Aug 08, 2017 at 07:36:25AM +0300, Roi Dayan wrote: > >> > >> > >>On 07/08/2017 20:05, Ben Pfaff wrote: > >>>On Mon, Aug 07, 2017 at 07:00:31AM -0700, Ben Pfaff wrote: > >>>>On Mon, Aug 07, 2017 at 07:32:02AM +0300, Roi Dayan wrote: > >>>>>From: Paul Blakey > >>>>> > >>>>>Always implement get_ifindex without checking if offload is > >>>>>enabled or not as this should not be related. From ovs-dpctl > >>>>>we cannot tell if offload is enabled or not as other_config is > >>>>>not being read. > >>>>> > >>>>>Signed-off-by: Paul Blakey > >>>>>Reviewed-by: Roi Dayan > >>>> > >>>>Applied to master and branch-2.8, thanks! > >>> > >>>Sorry, I had to revert this because it caused several unit test > >>>failures: 770 781 783 787 788 791 2189 2378. > >>> > >> > >>This is because of the warnings from get_ifindex which resolved in > >>the second patch but was missing the ratelimiting you mentioned. > >>I submitted V2 of it to add back the ratelimiting > >>"netdev-linux: Reduce log level for ENODEV errors getting ifindex" > > > >In that case shouldn't the patch order be reversed to avoid > >the (temporary) regression Ben pointed out? > > > > right. > should i post V3 for changing the order or is it something that > can be done when merged? I guess its up to Ben, but surely posting v3 would not hurt. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/7] Add offload support for ip ttl and tcp flags
On Fri, Aug 11, 2017 at 11:43:23AM -0700, Joe Stringer wrote:
> On 8 August 2017 at 01:03, Simon Horman wrote:
> > On Mon, Aug 07, 2017 at 06:19:04PM +0300, Roi Dayan wrote:
> >> Hi,
> >>
> >> This series adds support for offloading ip ttl and tcp flags
> >> using tc interface.
> >
> > This looks nice, thanks.
> >
> > Acked-by: Simon Horman
> >
> > I'm also happy to apply this if someone else provides a review.
>
> Thanks folks, I'll apply the series to master soon with Simon's acks
> and the following style fixup.
Thanks.
> diff --git a/lib/tc.c b/lib/tc.c
> index bf12a5bea494..c9cada249de3 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -363,7 +363,6 @@ nl_parse_flower_ip(struct nlattr **attrs, struct
> tc_flower *flower) {
> key->ip_ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TTL]);
> mask->ip_ttl = nl_attr_get_u8(attrs[TCA_FLOWER_KEY_IP_TTL_MASK]);
> }
> -
> }
>
> static const struct nl_policy tunnel_key_policy[] = {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action
On Tue, Aug 08, 2017 at 05:21:53PM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> To be later used to implement ovs action set offloading.
>
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
> ---
> lib/tc.c | 372
> ++-
> lib/tc.h | 12 +++
> 2 files changed, 381 insertions(+), 3 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
...
> @@ -589,6 +758,10 @@ nl_parse_single_action(struct nlattr *action, struct
> tc_flower *flower)
> nl_parse_act_vlan(act_options, flower);
> } else if (!strcmp(act_kind, "tunnel_key")) {
> nl_parse_act_tunnel_key(act_options, flower);
> +} else if (!strcmp(act_kind, "pedit")) {
> +nl_parse_act_pedit(act_options, flower);
> +} else if (!strcmp(act_kind, "csum")) {
> +/* not doing anything for now */
> } else {
> VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind);
> return EINVAL;
> @@ -790,6 +963,48 @@ tc_get_tc_cls_policy(enum tc_offload_policy policy)
> }
>
> static void
> +nl_msg_put_act_csum(struct ofpbuf *request, uint32_t flags)
> +{
> +size_t offset;
> +
> +nl_msg_put_string(request, TCA_ACT_KIND, "csum");
> +offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> +{
> +struct tc_csum parm = { .action = TC_ACT_PIPE,
> +.update_flags = flags };
I think it would be a bit nicer to move param to the top of the function
and remove the extra { } scope it is currently inside.
> +
> +nl_msg_put_unspec(request, TCA_CSUM_PARMS, &parm, sizeof parm);
> +}
> +nl_msg_end_nested(request, offset);
> +}
> +
> +static void
> +nl_msg_put_act_pedit(struct ofpbuf *request, struct tc_pedit *parm,
> + struct tc_pedit_key_ex *ex)
> +{
> +size_t ksize = sizeof *parm + (parm->nkeys * sizeof(struct
> tc_pedit_key));
> +size_t offset, offset_keys_ex, offset_key;
> +int i;
> +
> +nl_msg_put_string(request, TCA_ACT_KIND, "pedit");
> +offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS);
> +{
The extra { } scope here seems unnecessary.
> +parm->action = TC_ACT_PIPE;
> +
> +nl_msg_put_unspec(request, TCA_PEDIT_PARMS_EX, parm, ksize);
> +offset_keys_ex = nl_msg_start_nested(request, TCA_PEDIT_KEYS_EX);
> +for (i = 0; i < parm->nkeys; i++, ex++) {
> +offset_key = nl_msg_start_nested(request, TCA_PEDIT_KEY_EX);
> +nl_msg_put_u16(request, TCA_PEDIT_KEY_EX_HTYPE, ex->htype);
> +nl_msg_put_u16(request, TCA_PEDIT_KEY_EX_CMD, ex->cmd);
> +nl_msg_end_nested(request, offset_key);
> +}
> +nl_msg_end_nested(request, offset_keys_ex);
> +}
> +nl_msg_end_nested(request, offset);
> +}
> +
> +static void
> nl_msg_put_act_push_vlan(struct ofpbuf *request, uint16_t vid, uint8_t prio)
> {
> size_t offset;
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/4] Add offload support for action set
On Tue, Aug 08, 2017 at 05:21:50PM +0300, Roi Dayan wrote: > Hi, > > This series adds support for offloading action set using > tc interface. Other than some minor nits regarding the 3rd patch, which I posted in response to that patch, this series looks good to me. And this is a feature I am very pleased to see. Acked-by: Simon Horman > > Thanks, > Roi > > > Paul Blakey (4): > compat: Add act_pedit compatibility for old kernels > odp-util: Expose ovs flow key attr len table for reuse > tc: Add header rewrite using tc pedit action > netdev-tc-offloads: Add support for action set ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH/RFC] bond: add enable-recirc configuration for bond.
From: Pieter Jansen van Vuuren
Adds a config parameter that determines if a bond will use recirculation
in the kernel datapath when implementing a bond in balance-tcp mode.
The default for enable-recirc is "true", resulting in the traditional
implementation of a bond in balance-tcp mode. Setting enable-recirc to
false results in datapath rules that do not rely on the recirculation
action.
example usage:
ovs-vsctl set port bond0 other_config:enable-recirc=false
Advantages:
- Allows TC offloading of OVS bonds on hardware which
does not support recirculation
- Appears to result in lower latency (in systems using few flows).
Quick ping test results in:
other_config:enable-recirc=false
rtt min/avg/max/mdev = 0.039/0.193/7.612/1.059 ms
other_config:enable-recirc=true
rtt min/avg/max/mdev = 0.038/0.321/14.091/1.967 ms
More comprehensive testing is in progress.
Signed-off-by: Pieter Jansen van Vuuren
Signed-off-by: Simon Horman
---
ofproto/bond.c | 11 ++-
ofproto/bond.h | 1 +
vswitchd/bridge.c| 3 +++
vswitchd/vswitch.xml | 8
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index 365a3ca7ffad..46f8a9afcb3b 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -147,6 +147,7 @@ struct bond {
/* The MAC address of the active interface. */
/* Legacy compatibility. */
bool lacp_fallback_ab; /* Fallback to active-backup on LACP failure. */
+bool recirc_enabled;
struct ovs_refcount ref_cnt;
};
@@ -437,6 +438,11 @@ bond_reconfigure(struct bond *bond, const struct
bond_settings *s)
revalidate = true;
}
+if (bond->recirc_enabled != s->recirc_enabled) {
+bond->recirc_enabled = s->recirc_enabled;
+revalidate = true;
+}
+
if (bond->rebalance_interval != s->rebalance_interval) {
bond->rebalance_interval = s->rebalance_interval;
revalidate = true;
@@ -458,7 +464,10 @@ bond_reconfigure(struct bond *bond, const struct
bond_settings *s)
}
if (bond->balance != BM_AB) {
-if (!bond->recirc_id) {
+ if (!bond->recirc_enabled) {
+recirc_free_id(bond->recirc_id);
+bond->recirc_id = 0;
+} else if (!bond->recirc_id) {
bond->recirc_id = recirc_alloc_id(bond->ofproto);
}
} else if (bond->recirc_id) {
diff --git a/ofproto/bond.h b/ofproto/bond.h
index e7c3d9bc35dd..beb937b9910e 100644
--- a/ofproto/bond.h
+++ b/ofproto/bond.h
@@ -53,6 +53,7 @@ struct bond_settings {
int down_delay; /* ms before disabling a down slave. */
bool lacp_fallback_ab_cfg; /* Fallback to active-backup on LACP failure.
*/
+bool recirc_enabled;
struct eth_addr active_slave_mac;
/* The MAC address of the interface
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index a8cbae78cb23..b4b5c89ca6a0 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -4259,6 +4259,9 @@ port_configure_bond(struct port *port, struct
bond_settings *s)
s->lacp_fallback_ab_cfg = smap_get_bool(&port->cfg->other_config,
"lacp-fallback-ab", false);
+s->recirc_enabled = smap_get_bool(&port->cfg->other_config,
+ "enable-recirc", true);
+
LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
netdev_set_miimon_interval(iface->netdev, miimon_interval);
}
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 074535b588ef..7b97d720d276 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -1693,6 +1693,14 @@
is configured to LACP mode, the bond will use LACP.
+
+
+
+Determines if a bond will use recirculation in the kernel datapath
+when implementing a bond in balance-tcp mode.
+
+
--
2.1.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH/RFC] bond: add enable-recirc configuration for bond.
Hi Andy, On Tue, Aug 15, 2017 at 01:31:36PM -0700, Andy Zhou wrote: > On Tue, Aug 15, 2017 at 9:13 AM, Simon Horman > wrote: > > From: Pieter Jansen van Vuuren > > > > Adds a config parameter that determines if a bond will use recirculation > > in the kernel datapath when implementing a bond in balance-tcp mode. > > Using recirc for bonding is a lower level decision. I am not sure > OVSDB is right level > for exposing this detail. I do see that this configuration is arguably more detailed than existing bond parameters set via OVSDB, but it is still a bond parameter so perhaps it isn't entirely out of place here. > > The default for enable-recirc is "true", resulting in the traditional > > implementation of a bond in balance-tcp mode. Setting enable-recirc to > > false results in datapath rules that do not rely on the recirculation > > action. > > > > example usage: > > ovs-vsctl set port bond0 other_config:enable-recirc=false > > > > Advantages: > > - Allows TC offloading of OVS bonds on hardware which > > does not support recirculation > > Not sure it is an advantage. Bonding with recirc allows the first flow to > be a mega flow. without megaflow, all bond traffic has to be micro > flows, which was how OVS implements bonding. The recirc was introduced to > solve the flow explosion problem > > Without supporting recirc, I'd image TC offloading would suffer the same > issue. May be it is correct not to offload the bond flows, at least not > before recirc is supported in TC. > > Another way to look at this is that when TC ran out of flow space, and > bonding is supported by the kernel module. This configuration would > prevent kernel module from using recirc. I believe that your points regarding flow explosion and fallback to the kernel datapath are well made. However, I also believe there is hardware where using recirculation is not practical. For that reason I think it is worth exploring this proposal further. > > - Appears to result in lower latency (in systems using few flows). > > Not sure this is true in general. When using recirc with proper > megaflow, the latency should > improve for new microflows since upcalls can be omitted. That may be true in terms of flow setup cost but I believe that the per-packet cost is going to be higher when recirculation is used. > > Quick ping test results in: > > > > other_config:enable-recirc=false > > rtt min/avg/max/mdev = 0.039/0.193/7.612/1.059 ms > > > > other_config:enable-recirc=true > > rtt min/avg/max/mdev = 0.038/0.321/14.091/1.967 ms > > > > When setting up the flow, 'recirc" requires 2 miss upcalls, thus set > up latency is larger. > However, once flows are set up, the latency difference should be minimal. > > I'd expect the max latency to be different with or without recirc. > Notice the minimal latency difference is small. > I'd expect the average latency to converge to minimal for sufficiently > large number of ping packets. Latency may not be the best way to measure this but having to classify each packet twice, as I believe is the case when recirculation is used, must cost something. > > More comprehensive testing is in progress. > > > > Signed-off-by: Pieter Jansen van Vuuren > > > > Signed-off-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action
On Thu, Aug 17, 2017 at 12:21:43PM +0300, Paul Blakey wrote:
>
>
> On 14/08/2017 16:00, Simon Horman wrote:
> >On Tue, Aug 08, 2017 at 05:21:53PM +0300, Roi Dayan wrote:
> >>From: Paul Blakey
> >>
> >>To be later used to implement ovs action set offloading.
> >>
> >>Signed-off-by: Paul Blakey
> >>Reviewed-by: Roi Dayan
> >>---
> >> lib/tc.c | 372
> >> ++-
> >> lib/tc.h | 12 +++
> >> 2 files changed, 381 insertions(+), 3 deletions(-)
...
> >>+static void
> >> nl_msg_put_act_push_vlan(struct ofpbuf *request, uint16_t vid, uint8_t
> >> prio)
> >> {
> >> size_t offset;
>
> Hi and thanks for the review,
>
> The above style corresponds with the rest of the file (scope and
> nl_msg_put_act_* funcs)
> Maybe we do a style patch that remove all this extra scopes in a later
> patch?
Sure, that is fine by me.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] lib/odp: Fix handling of set masked action in parse_odp_action
On Tue, Aug 29, 2017 at 07:30:23AM +0300, Roi Dayan wrote:
> From: Paul Blakey
>
> If we find that we need to change from a SET to SET_MASKED action,
> then we write the mask to the actions opfbuf. But if there was netlink
> pad added to the buffer when writing the key, mask won't follow the
> key data as per SET_MASKED spec.
>
> Fix that by removing the padding before writing the mask, and
> readding it if needed for alignment.
>
> Fixes: 6d670e7f0d45 ("lib/odp: Masked set action execution and printing.")
> Signed-off-by: Paul Blakey
> Reviewed-by: Roi Dayan
Some minor nits below but that notwithstanding
Acked-by: Simon Horman
> ---
> lib/odp-util.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 4f1499e..0594840 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -1990,8 +1990,17 @@ parse_odp_action(const char *s, const struct simap
> *port_names,
> if (size == nl_attr_get_size(key)) {
> /* Change to masked set action if not fully masked. */
> if (!is_all_ones(mask + 1, size)) {
> +/* remove padding of eariler key payload */
> +actions->size -= NLA_ALIGN(key->nla_len) - key->nla_len;
> +
> +/* put mask payload right after key payload */
> key->nla_len += size;
> ofpbuf_put(actions, mask + 1, size);
> +
> +/* add back the netlink padding, if needed */
Maybe:
/* Add new padding as needed */
> +ofpbuf_put_zeros(actions, NLA_ALIGN(key->nla_len) -
> + key->nla_len);
The indentation of the above seems wrong:
+ofpbuf_put_zeros(actions, NLA_ALIGN(key->nla_len) -
+ key->nla_len);
> +
> /* 'actions' may have been reallocated by ofpbuf_put(). */
> nested = ofpbuf_at_assert(actions, start_ofs, sizeof
> *nested);
> nested->nla_type = OVS_ACTION_ATTR_SET_MASKED;
> --
> 2.7.0
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
