Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Tue, Jun 2, 2020 at 5:34 PM Simon Horman wrote:
>
> On Tue, Jun 02, 2020 at 04:58:32PM +0800, Tonghao Zhang wrote:
> > On Tue, Jun 2, 2020 at 4:33 PM Simon Horman
> > wrote:
> > >
> > > On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> > > > On Fri, May 29, 2020 at 5:06 PM Simon Horman
> > > > wrote:
> > > > >
> > > > > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m@gmail.com
> > > > > wrote:
> > > > > > From: Tonghao Zhang
> > > > > >
> > > > > > This patch allows users to offload the TC flower rules with tunnel
> > > > > > mask. In some case, mask is useful as wildcards.
> > > > > >
> > > > > > For example:
> > > > > > $ ovs-appctl dpctl/add-flow \
> > > > > > "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > > > > recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> > > > >
> > > > > Hi,
> > > > >
> > > > > Sorry for the delay in responding.
> > > > >
> > > > > I think it would be useful to spell out more clearly in the changelog
> > > > > what this patch does. From my reading of the code it:
> > > > >
> > > > > Allows masked match of the following, where previously supported
> > > > > an exact match was supported:
> > > > > * Remote (dst) tunnel endpoint address
> > > > > * Local (src) tunnel endpoint address
> > > > > * Remote (dst) tunnel endpoint UDP port
> > > > >
> > > > > And also allows masked match of the following, where previously no
> > > > > match was supported;
> > > > > * Local (std) tunnel endpoint UDP port
> > > > Ok, Thanks, I will update it in NEWS.
> > >
> > > Thanks. Please also include this information in the changelog.
> > >
> > > > > I think it would also be useful to describe a use-case where this
> > > > > is used. The command line example (above) is a good start.
> > > > Yes, I will update the commit log and describe a use-case for it.
> > > > >
> > > > > Also, not strictly related to this patch.
> > > > > I think patch series that have more than one patch should
> > > > > have a cover letter, in this case [PATCH 0/3], describing
> > > > > the overall aim of the patchset.
> > > > >
> > > > >
> > > > > The other patches in this series seem fine to me.
> > > > > Please consider addressing the issues I have raised here
> > > > > and posting a v2, with all three patches and a cover letter.
> > > > >
> > > > > ...
> > > > >
> > > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > > > index 875ebef71941..39cf25f63ce0 100644
> > > > > > --- a/lib/netdev-offload-tc.c
> > > > > > +++ b/lib/netdev-offload-tc.c
> > > > > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower
> > > > > > *flower,
> > > > > > match_set_tun_id(match, flower->key.tunnel.id);
> > > > > > match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > > > > }
> > > > > > -if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > > > > -match_set_tun_src(match,
> > > > > > flower->key.tunnel.ipv4.ipv4_src);
> > > > > > -match_set_tun_dst(match,
> > > > > > flower->key.tunnel.ipv4.ipv4_dst);
> > > > > > -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> > > > > > - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > > > > -match_set_tun_ipv6_src(match,
> > > > > > >key.tunnel.ipv6.ipv6_src);
> > > > > > -match_set_tun_ipv6_dst(match,
> > > > > > >key.tunnel.ipv6.ipv6_dst);
> > > > > > +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > > > > +flower->mask.tunnel.ipv4.ipv4_src) {
> > > > >
> > > > > The change to the if condition seems separate from the change
> > > > > described in the changelog. What is the use-case for this?
> > > > I think that may be used for matching the tunnel src packets only
> > > > which will be dropped.
> > > > because user may don't want that packets sent to the host.
> > >
> > > I think it would be best to break out this (and the corresponding IPv6
> > > change) into a separate patch with a changelog that describes why
> > > this is being done and, if appropriate, an update to NEWS.
> > Hi Simon
> > Did you mean that the two patches as below as a series.
> > [PATCH 1/3] dpif-netlink: Generate ufids for installing TC flowers
> > [PATCH 3/3] netdev-offload-tc: Use ipv6_addr_is_set instead of is_all_zeros
> >
> > and this patch as a separate patch(with changelog)
> > [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
>
> Sorry for not being clear.
>
> I meant expand the series to four patch.
> Where the new patch just includes the following changes:
>
> -if (flower->key.tunnel.ipv4.ipv4_dst) {
> +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> +flower->mask.tunnel.ipv4.ipv4_src) {
>
> ...
>
> -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> +} else if (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst) ||
> +
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Tue, Jun 02, 2020 at 04:58:32PM +0800, Tonghao Zhang wrote:
> On Tue, Jun 2, 2020 at 4:33 PM Simon Horman
> wrote:
> >
> > On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> > > On Fri, May 29, 2020 at 5:06 PM Simon Horman
> > > wrote:
> > > >
> > > > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m@gmail.com
> > > > wrote:
> > > > > From: Tonghao Zhang
> > > > >
> > > > > This patch allows users to offload the TC flower rules with tunnel
> > > > > mask. In some case, mask is useful as wildcards.
> > > > >
> > > > > For example:
> > > > > $ ovs-appctl dpctl/add-flow \
> > > > > "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > > > recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> > > >
> > > > Hi,
> > > >
> > > > Sorry for the delay in responding.
> > > >
> > > > I think it would be useful to spell out more clearly in the changelog
> > > > what this patch does. From my reading of the code it:
> > > >
> > > > Allows masked match of the following, where previously supported
> > > > an exact match was supported:
> > > > * Remote (dst) tunnel endpoint address
> > > > * Local (src) tunnel endpoint address
> > > > * Remote (dst) tunnel endpoint UDP port
> > > >
> > > > And also allows masked match of the following, where previously no
> > > > match was supported;
> > > > * Local (std) tunnel endpoint UDP port
> > > Ok, Thanks, I will update it in NEWS.
> >
> > Thanks. Please also include this information in the changelog.
> >
> > > > I think it would also be useful to describe a use-case where this
> > > > is used. The command line example (above) is a good start.
> > > Yes, I will update the commit log and describe a use-case for it.
> > > >
> > > > Also, not strictly related to this patch.
> > > > I think patch series that have more than one patch should
> > > > have a cover letter, in this case [PATCH 0/3], describing
> > > > the overall aim of the patchset.
> > > >
> > > >
> > > > The other patches in this series seem fine to me.
> > > > Please consider addressing the issues I have raised here
> > > > and posting a v2, with all three patches and a cover letter.
> > > >
> > > > ...
> > > >
> > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > > index 875ebef71941..39cf25f63ce0 100644
> > > > > --- a/lib/netdev-offload-tc.c
> > > > > +++ b/lib/netdev-offload-tc.c
> > > > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower
> > > > > *flower,
> > > > > match_set_tun_id(match, flower->key.tunnel.id);
> > > > > match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > > > }
> > > > > -if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > > > -match_set_tun_src(match,
> > > > > flower->key.tunnel.ipv4.ipv4_src);
> > > > > -match_set_tun_dst(match,
> > > > > flower->key.tunnel.ipv4.ipv4_dst);
> > > > > -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> > > > > - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > > > -match_set_tun_ipv6_src(match,
> > > > > >key.tunnel.ipv6.ipv6_src);
> > > > > -match_set_tun_ipv6_dst(match,
> > > > > >key.tunnel.ipv6.ipv6_dst);
> > > > > +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > > > +flower->mask.tunnel.ipv4.ipv4_src) {
> > > >
> > > > The change to the if condition seems separate from the change
> > > > described in the changelog. What is the use-case for this?
> > > I think that may be used for matching the tunnel src packets only
> > > which will be dropped.
> > > because user may don't want that packets sent to the host.
> >
> > I think it would be best to break out this (and the corresponding IPv6
> > change) into a separate patch with a changelog that describes why
> > this is being done and, if appropriate, an update to NEWS.
> Hi Simon
> Did you mean that the two patches as below as a series.
> [PATCH 1/3] dpif-netlink: Generate ufids for installing TC flowers
> [PATCH 3/3] netdev-offload-tc: Use ipv6_addr_is_set instead of is_all_zeros
>
> and this patch as a separate patch(with changelog)
> [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
Sorry for not being clear.
I meant expand the series to four patch.
Where the new patch just includes the following changes:
-if (flower->key.tunnel.ipv4.ipv4_dst) {
+if (flower->mask.tunnel.ipv4.ipv4_dst ||
+flower->mask.tunnel.ipv4.ipv4_src) {
...
-} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
- sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
+} else if (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst) ||
+ ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_src)) {
>
> and other question about changelog, which file I should update, or I just
> update
> it in a commit message.
Sorry again for not being clear, I meant the commit message.
>
> I found the changelog in: debian/changelog
> >
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Tue, Jun 2, 2020 at 4:33 PM Simon Horman wrote:
>
> On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> > On Fri, May 29, 2020 at 5:06 PM Simon Horman
> > wrote:
> > >
> > > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m@gmail.com wrote:
> > > > From: Tonghao Zhang
> > > >
> > > > This patch allows users to offload the TC flower rules with tunnel
> > > > mask. In some case, mask is useful as wildcards.
> > > >
> > > > For example:
> > > > $ ovs-appctl dpctl/add-flow \
> > > > "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > > recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> > >
> > > Hi,
> > >
> > > Sorry for the delay in responding.
> > >
> > > I think it would be useful to spell out more clearly in the changelog
> > > what this patch does. From my reading of the code it:
> > >
> > > Allows masked match of the following, where previously supported
> > > an exact match was supported:
> > > * Remote (dst) tunnel endpoint address
> > > * Local (src) tunnel endpoint address
> > > * Remote (dst) tunnel endpoint UDP port
> > >
> > > And also allows masked match of the following, where previously no
> > > match was supported;
> > > * Local (std) tunnel endpoint UDP port
> > Ok, Thanks, I will update it in NEWS.
>
> Thanks. Please also include this information in the changelog.
>
> > > I think it would also be useful to describe a use-case where this
> > > is used. The command line example (above) is a good start.
> > Yes, I will update the commit log and describe a use-case for it.
> > >
> > > Also, not strictly related to this patch.
> > > I think patch series that have more than one patch should
> > > have a cover letter, in this case [PATCH 0/3], describing
> > > the overall aim of the patchset.
> > >
> > >
> > > The other patches in this series seem fine to me.
> > > Please consider addressing the issues I have raised here
> > > and posting a v2, with all three patches and a cover letter.
> > >
> > > ...
> > >
> > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > > index 875ebef71941..39cf25f63ce0 100644
> > > > --- a/lib/netdev-offload-tc.c
> > > > +++ b/lib/netdev-offload-tc.c
> > > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > > > match_set_tun_id(match, flower->key.tunnel.id);
> > > > match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > > }
> > > > -if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > > -match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > > > -match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > > > -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> > > > - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > > -match_set_tun_ipv6_src(match,
> > > > >key.tunnel.ipv6.ipv6_src);
> > > > -match_set_tun_ipv6_dst(match,
> > > > >key.tunnel.ipv6.ipv6_dst);
> > > > +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > > +flower->mask.tunnel.ipv4.ipv4_src) {
> > >
> > > The change to the if condition seems separate from the change
> > > described in the changelog. What is the use-case for this?
> > I think that may be used for matching the tunnel src packets only
> > which will be dropped.
> > because user may don't want that packets sent to the host.
>
> I think it would be best to break out this (and the corresponding IPv6
> change) into a separate patch with a changelog that describes why
> this is being done and, if appropriate, an update to NEWS.
Hi Simon
Did you mean that the two patches as below as a series.
[PATCH 1/3] dpif-netlink: Generate ufids for installing TC flowers
[PATCH 3/3] netdev-offload-tc: Use ipv6_addr_is_set instead of is_all_zeros
and this patch as a separate patch(with changelog)
[PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
and other question about changelog, which file I should update, or I just update
it in a commit message.
I found the changelog in: debian/changelog
> Thanks!
>
> > > > +match_set_tun_dst_masked(match,
> > > > + flower->key.tunnel.ipv4.ipv4_dst,
> > > > +
> > > > flower->mask.tunnel.ipv4.ipv4_dst);
> > > > +match_set_tun_src_masked(match,
> > > > + flower->key.tunnel.ipv4.ipv4_src,
> > > > +
> > > > flower->mask.tunnel.ipv4.ipv4_src);
> > > > +} else if
> > > > (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst) ||
> > > > +
> > > > ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_src)) {
> > > > +match_set_tun_ipv6_dst_masked(match,
> > > > +
> > > > >key.tunnel.ipv6.ipv6_dst,
> > > > +
> > > > >mask.tunnel.ipv6.ipv6_dst);
> > > > +
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Tue, Jun 02, 2020 at 04:17:21PM +0800, Tonghao Zhang wrote:
> On Fri, May 29, 2020 at 5:06 PM Simon Horman
> wrote:
> >
> > On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m@gmail.com wrote:
> > > From: Tonghao Zhang
> > >
> > > This patch allows users to offload the TC flower rules with tunnel
> > > mask. In some case, mask is useful as wildcards.
> > >
> > > For example:
> > > $ ovs-appctl dpctl/add-flow \
> > > "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > > recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
> >
> > Hi,
> >
> > Sorry for the delay in responding.
> >
> > I think it would be useful to spell out more clearly in the changelog
> > what this patch does. From my reading of the code it:
> >
> > Allows masked match of the following, where previously supported
> > an exact match was supported:
> > * Remote (dst) tunnel endpoint address
> > * Local (src) tunnel endpoint address
> > * Remote (dst) tunnel endpoint UDP port
> >
> > And also allows masked match of the following, where previously no
> > match was supported;
> > * Local (std) tunnel endpoint UDP port
> Ok, Thanks, I will update it in NEWS.
Thanks. Please also include this information in the changelog.
> > I think it would also be useful to describe a use-case where this
> > is used. The command line example (above) is a good start.
> Yes, I will update the commit log and describe a use-case for it.
> >
> > Also, not strictly related to this patch.
> > I think patch series that have more than one patch should
> > have a cover letter, in this case [PATCH 0/3], describing
> > the overall aim of the patchset.
> >
> >
> > The other patches in this series seem fine to me.
> > Please consider addressing the issues I have raised here
> > and posting a v2, with all three patches and a cover letter.
> >
> > ...
> >
> > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > > index 875ebef71941..39cf25f63ce0 100644
> > > --- a/lib/netdev-offload-tc.c
> > > +++ b/lib/netdev-offload-tc.c
> > > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > > match_set_tun_id(match, flower->key.tunnel.id);
> > > match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > > }
> > > -if (flower->key.tunnel.ipv4.ipv4_dst) {
> > > -match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > > -match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > > -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> > > - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > > -match_set_tun_ipv6_src(match,
> > > >key.tunnel.ipv6.ipv6_src);
> > > -match_set_tun_ipv6_dst(match,
> > > >key.tunnel.ipv6.ipv6_dst);
> > > +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > > +flower->mask.tunnel.ipv4.ipv4_src) {
> >
> > The change to the if condition seems separate from the change
> > described in the changelog. What is the use-case for this?
> I think that may be used for matching the tunnel src packets only
> which will be dropped.
> because user may don't want that packets sent to the host.
I think it would be best to break out this (and the corresponding IPv6
change) into a separate patch with a changelog that describes why
this is being done and, if appropriate, an update to NEWS.
Thanks!
> > > +match_set_tun_dst_masked(match,
> > > + flower->key.tunnel.ipv4.ipv4_dst,
> > > + flower->mask.tunnel.ipv4.ipv4_dst);
> > > +match_set_tun_src_masked(match,
> > > + flower->key.tunnel.ipv4.ipv4_src,
> > > + flower->mask.tunnel.ipv4.ipv4_src);
> > > +} else if (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst)
> > > ||
> > > + ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_src))
> > > {
> > > +match_set_tun_ipv6_dst_masked(match,
> > > +
> > > >key.tunnel.ipv6.ipv6_dst,
> > > +
> > > >mask.tunnel.ipv6.ipv6_dst);
> > > +match_set_tun_ipv6_src_masked(match,
> > > +
> > > >key.tunnel.ipv6.ipv6_src,
> > > +
> > > >mask.tunnel.ipv6.ipv6_src);
> > > }
> > > if (flower->key.tunnel.tos) {
> > > match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> >
> > ...
>
>
>
> --
> Best regards, Tonghao
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Fri, May 29, 2020 at 5:06 PM Simon Horman wrote:
>
> On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m@gmail.com wrote:
> > From: Tonghao Zhang
> >
> > This patch allows users to offload the TC flower rules with tunnel
> > mask. In some case, mask is useful as wildcards.
> >
> > For example:
> > $ ovs-appctl dpctl/add-flow \
> > "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> > recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
>
> Hi,
>
> Sorry for the delay in responding.
>
> I think it would be useful to spell out more clearly in the changelog
> what this patch does. From my reading of the code it:
>
> Allows masked match of the following, where previously supported
> an exact match was supported:
> * Remote (dst) tunnel endpoint address
> * Local (src) tunnel endpoint address
> * Remote (dst) tunnel endpoint UDP port
>
> And also allows masked match of the following, where previously no
> match was supported;
> * Local (std) tunnel endpoint UDP port
Ok, Thanks, I will update it in NEWS.
> I think it would also be useful to describe a use-case where this
> is used. The command line example (above) is a good start.
Yes, I will update the commit log and describe a use-case for it.
>
> Also, not strictly related to this patch.
> I think patch series that have more than one patch should
> have a cover letter, in this case [PATCH 0/3], describing
> the overall aim of the patchset.
>
>
> The other patches in this series seem fine to me.
> Please consider addressing the issues I have raised here
> and posting a v2, with all three patches and a cover letter.
>
> ...
>
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> > index 875ebef71941..39cf25f63ce0 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> > match_set_tun_id(match, flower->key.tunnel.id);
> > match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> > }
> > -if (flower->key.tunnel.ipv4.ipv4_dst) {
> > -match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> > -match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> > -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> > - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> > -match_set_tun_ipv6_src(match,
> > >key.tunnel.ipv6.ipv6_src);
> > -match_set_tun_ipv6_dst(match,
> > >key.tunnel.ipv6.ipv6_dst);
> > +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> > +flower->mask.tunnel.ipv4.ipv4_src) {
>
> The change to the if condition seems separate from the change
> described in the changelog. What is the use-case for this?
I think that may be used for matching the tunnel src packets only
which will be dropped.
because user may don't want that packets sent to the host.
> > +match_set_tun_dst_masked(match,
> > + flower->key.tunnel.ipv4.ipv4_dst,
> > + flower->mask.tunnel.ipv4.ipv4_dst);
> > +match_set_tun_src_masked(match,
> > + flower->key.tunnel.ipv4.ipv4_src,
> > + flower->mask.tunnel.ipv4.ipv4_src);
> > +} else if (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst) ||
> > + ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_src)) {
> > +match_set_tun_ipv6_dst_masked(match,
> > +
> > >key.tunnel.ipv6.ipv6_dst,
> > +
> > >mask.tunnel.ipv6.ipv6_dst);
> > +match_set_tun_ipv6_src_masked(match,
> > +
> > >key.tunnel.ipv6.ipv6_src,
> > +
> > >mask.tunnel.ipv6.ipv6_src);
> > }
> > if (flower->key.tunnel.tos) {
> > match_set_tun_tos_masked(match, flower->key.tunnel.tos,
>
> ...
--
Best regards, Tonghao
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Sun, May 17, 2020 at 10:02:05PM -0400, 0-day Robot wrote: > Bleep bloop. Greetings Tonghao Zhang, I am a robot and I have tried out your > patch. > Thanks for your contribution. > > I encountered some error that I wasn't expecting. See the details below. > > > checkpatch: > WARNING: Line is 84 characters long (recommended limit is 79) > #38 FILE: include/openvswitch/match.h:109: > void match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 > mask); > > Lines checked: 288, Warnings: 1, Errors: 0 > > > Please check this out. If you feel there has been an error, please email > acon...@redhat.com Please consider addressing this (minor) issue when posting v2 of this series. Thanks! ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On Mon, May 18, 2020 at 09:44:42AM +0800, xiangxia.m@gmail.com wrote:
> From: Tonghao Zhang
>
> This patch allows users to offload the TC flower rules with tunnel
> mask. In some case, mask is useful as wildcards.
>
> For example:
> $ ovs-appctl dpctl/add-flow \
> "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
Hi,
Sorry for the delay in responding.
I think it would be useful to spell out more clearly in the changelog
what this patch does. From my reading of the code it:
Allows masked match of the following, where previously supported
an exact match was supported:
* Remote (dst) tunnel endpoint address
* Local (src) tunnel endpoint address
* Remote (dst) tunnel endpoint UDP port
And also allows masked match of the following, where previously no
match was supported;
* Local (std) tunnel endpoint UDP port
I think it would also be useful to describe a use-case where this
is used. The command line example (above) is a good start.
Also, not strictly related to this patch.
I think patch series that have more than one patch should
have a cover letter, in this case [PATCH 0/3], describing
the overall aim of the patchset.
The other patches in this series seem fine to me.
Please consider addressing the issues I have raised here
and posting a v2, with all three patches and a cover letter.
...
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 875ebef71941..39cf25f63ce0 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> match_set_tun_id(match, flower->key.tunnel.id);
> match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> }
> -if (flower->key.tunnel.ipv4.ipv4_dst) {
> -match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> -match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> -match_set_tun_ipv6_src(match, >key.tunnel.ipv6.ipv6_src);
> -match_set_tun_ipv6_dst(match, >key.tunnel.ipv6.ipv6_dst);
> +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> +flower->mask.tunnel.ipv4.ipv4_src) {
The change to the if condition seems separate from the change
described in the changelog. What is the use-case for this?
> +match_set_tun_dst_masked(match,
> + flower->key.tunnel.ipv4.ipv4_dst,
> + flower->mask.tunnel.ipv4.ipv4_dst);
> +match_set_tun_src_masked(match,
> + flower->key.tunnel.ipv4.ipv4_src,
> + flower->mask.tunnel.ipv4.ipv4_src);
> +} else if (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst) ||
> + ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_src)) {
> +match_set_tun_ipv6_dst_masked(match,
> + >key.tunnel.ipv6.ipv6_dst,
> +
> >mask.tunnel.ipv6.ipv6_dst);
> +match_set_tun_ipv6_src_masked(match,
> + >key.tunnel.ipv6.ipv6_src,
> +
> >mask.tunnel.ipv6.ipv6_src);
> }
> if (flower->key.tunnel.tos) {
> match_set_tun_tos_masked(match, flower->key.tunnel.tos,
...
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
On 2020-05-18 4:44 AM, xiangxia.m@gmail.com wrote:
> From: Tonghao Zhang
>
> This patch allows users to offload the TC flower rules with tunnel
> mask. In some case, mask is useful as wildcards.
>
> For example:
> $ ovs-appctl dpctl/add-flow \
> "tunnel(dst=3.3.3.100,src=3.3.3.200/255.255.255.0,tp_dst=4789),\
> recirc_id(0),in_port(3),eth(),eth_type(0x0800),ipv4()" "2"
>
> Cc: Simon Horman
> Cc: Paul Blakey
> Cc: Roi Dayan
> Cc: Ben Pfaff
> Cc: William Tu
> Cc: Ilya Maximets
> Signed-off-by: Tonghao Zhang
> ---
> include/openvswitch/match.h | 2 ++
> lib/match.c | 13 +
> lib/netdev-offload-tc.c | 40 --
> lib/tc.c| 57 +
> tests/tunnel.at | 22 ++
> 5 files changed, 119 insertions(+), 15 deletions(-)
>
> diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
> index 8af3b74ed3e0..cbb1a0382a2e 100644
> --- a/include/openvswitch/match.h
> +++ b/include/openvswitch/match.h
> @@ -105,6 +105,8 @@ void match_set_tun_flags(struct match *match, uint16_t
> flags);
> void match_set_tun_flags_masked(struct match *match, uint16_t flags,
> uint16_t mask);
> void match_set_tun_tp_dst(struct match *match, ovs_be16 tp_dst);
> void match_set_tun_tp_dst_masked(struct match *match, ovs_be16 port,
> ovs_be16 mask);
> +void match_set_tun_tp_src(struct match *match, ovs_be16 tp_src);
> +void match_set_tun_tp_src_masked(struct match *match, ovs_be16 port,
> ovs_be16 mask);
> void match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id,
> ovs_be16 mask);
> void match_set_tun_gbp_id(struct match *match, ovs_be16 gbp_id);
> void match_set_tun_gbp_flags_masked(struct match *match, uint8_t flags,
> uint8_t mask);
> diff --git a/lib/match.c b/lib/match.c
> index 25c277cc670b..29b25a73bab4 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -293,6 +293,19 @@ match_set_tun_tp_dst_masked(struct match *match,
> ovs_be16 port, ovs_be16 mask)
> match->flow.tunnel.tp_dst = port & mask;
> }
>
> +void
> +match_set_tun_tp_src(struct match *match, ovs_be16 tp_src)
> +{
> +match_set_tun_tp_src_masked(match, tp_src, OVS_BE16_MAX);
> +}
> +
> +void
> +match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16
> mask)
> +{
> +match->wc.masks.tunnel.tp_src = mask;
> +match->flow.tunnel.tp_src = port & mask;
> +}
> +
> void
> match_set_tun_gbp_id_masked(struct match *match, ovs_be16 gbp_id, ovs_be16
> mask)
> {
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 875ebef71941..39cf25f63ce0 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -633,13 +633,22 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> match_set_tun_id(match, flower->key.tunnel.id);
> match->flow.tunnel.flags |= FLOW_TNL_F_KEY;
> }
> -if (flower->key.tunnel.ipv4.ipv4_dst) {
> -match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
> -match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> -} else if (!is_all_zeros(>key.tunnel.ipv6.ipv6_dst,
> - sizeof flower->key.tunnel.ipv6.ipv6_dst)) {
> -match_set_tun_ipv6_src(match, >key.tunnel.ipv6.ipv6_src);
> -match_set_tun_ipv6_dst(match, >key.tunnel.ipv6.ipv6_dst);
> +if (flower->mask.tunnel.ipv4.ipv4_dst ||
> +flower->mask.tunnel.ipv4.ipv4_src) {
> +match_set_tun_dst_masked(match,
> + flower->key.tunnel.ipv4.ipv4_dst,
> + flower->mask.tunnel.ipv4.ipv4_dst);
> +match_set_tun_src_masked(match,
> + flower->key.tunnel.ipv4.ipv4_src,
> + flower->mask.tunnel.ipv4.ipv4_src);
> +} else if (ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_dst) ||
> + ipv6_addr_is_set(>mask.tunnel.ipv6.ipv6_src)) {
> +match_set_tun_ipv6_dst_masked(match,
> + >key.tunnel.ipv6.ipv6_dst,
> +
> >mask.tunnel.ipv6.ipv6_dst);
> +match_set_tun_ipv6_src_masked(match,
> + >key.tunnel.ipv6.ipv6_src,
> +
> >mask.tunnel.ipv6.ipv6_src);
> }
> if (flower->key.tunnel.tos) {
> match_set_tun_tos_masked(match, flower->key.tunnel.tos,
> @@ -649,8 +658,15 @@ parse_tc_flower_to_match(struct tc_flower *flower,
> match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
> flower->mask.tunnel.ttl);
> }
> -if (flower->key.tunnel.tp_dst) {
> -match_set_tun_tp_dst(match, flower->key.tunnel.tp_dst);
> +if (flower->mask.tunnel.tp_dst) {
> +
Re: [ovs-dev] [PATCH 2/3] netdev-offload-tc: Allow to match the IP and port mask of tunnel
Bleep bloop. Greetings Tonghao Zhang, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 84 characters long (recommended limit is 79) #38 FILE: include/openvswitch/match.h:109: void match_set_tun_tp_src_masked(struct match *match, ovs_be16 port, ovs_be16 mask); Lines checked: 288, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
