Re: [ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP
On 7 Jul 2023, at 14:25, Ilya Maximets wrote:
> On 7/7/23 14:11, Eelco Chaudron wrote:
>>
>>
>> On 7 Jul 2023, at 13:03, Ilya Maximets wrote:
>>
>>> On 7/6/23 11:30, Eelco Chaudron wrote:
On 30 Jun 2023, at 21:05, Eric Garver wrote:
Hi Eric,
I guess some description here would be good. Maybe add some info that this
is added due to kernel support being added.
In general, the patch looks good, and I would ack it. However, there is a
potential issue with TC hardware offload. If the patch gets applied as is,
and the kernel fix gets in, the additional DROP actions get added, but
they will not be offloaded to hardware and are only visible as kernel dp
entries.
I guess this might not be a real problem. However, people who upgrade OVS
with tc offload might get confused why they now get these drop rules in
the kernel dp. To solve this, we could simply skip adding them if hardware
offload is enabled. So the behavior stays the same. Something like this
(not tested, and we probably need to add dpif_is_netlink()):
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 29f4daa63..bc39e45bd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -34,12 +34,14 @@
#include "csum.h"
#include "dp-packet.h"
#include "dpif.h"
+#include "dpif-netdev.h"
#include "in-band.h"
#include "lacp.h"
#include "learn.h"
#include "mac-learning.h"
#include "mcast-snooping.h"
#include "multipath.h"
+#include "netdev-offload.h"
#include "netdev-vport.h"
#include "netlink.h"
#include "nx-match.h"
@@ -8199,7 +8201,8 @@ exit:
/* Install drop action if datapath supports explicit drop action. */
if (xin->odp_actions && !xin->odp_actions->size &&
-ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
+ovs_explicit_drop_action_supported(ctx.xbridge->ofproto) &&
+/* Do not add drop actions if TC hardware offload is enabled, as
+ * as TC does not support offloading the drop action. */
+(!netdev_is_flow_api_enabled() || dpif_is_netdev(xbridge->dpif)))
{
>>>
>>> I'd argue that the check should not be here. We should not set the
>>> baker.rt_support flag in the first place instead.
>>
>> Yes, that was my initial thought also, but what if hw offload gets enabled
>> later? We might need some re-evaluation at that stage. I know the docs say
>> you have to restart ovs when you enable HW offload, but that is not always
>> happening in practice.
>
> But if we keep it this way we'll have misleading information that it is
> supported in the log, while it will not be used in practice.
True, maybe we can re-evaluate if the hw offload flag changes?
>>
put_drop_action(xin->odp_actions, ctx.error);
}
I do not see a way for TC to support this as is in the kernel. We probably
need some new kind of drop reason supported TC action.
I know other people are exploring to see if this explicit drop action can
be used for other, more user-level things. If this is true this TC part
needs some proper fixing or we end up with rules not being offloaded.
Added some more people, who might have some opinions on TC offload / ideas
on how to add SKB_DROP_REASON support to TC.
Cheers,
Eelco
> Signed-off-by: Eric Garver
> ---
> include/linux/openvswitch.h | 2 +-
> lib/dpif.c | 6 --
> lib/dpif.h | 1 -
> ofproto/ofproto-dpif.c | 34 --
> 4 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index a265e05ad253..fce6456f47c8 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>
> #ifndef __KERNEL__
> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> #endif
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> diff --git a/lib/dpif.c b/lib/dpif.c
> index d328bf288de0..dc02ec912693 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const
Re: [ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP
On 7/7/23 14:11, Eelco Chaudron wrote:
>
>
> On 7 Jul 2023, at 13:03, Ilya Maximets wrote:
>
>> On 7/6/23 11:30, Eelco Chaudron wrote:
>>>
>>>
>>> On 30 Jun 2023, at 21:05, Eric Garver wrote:
>>>
>>> Hi Eric,
>>>
>>> I guess some description here would be good. Maybe add some info that this
>>> is added due to kernel support being added.
>>>
>>>
>>> In general, the patch looks good, and I would ack it. However, there is a
>>> potential issue with TC hardware offload. If the patch gets applied as is,
>>> and the kernel fix gets in, the additional DROP actions get added, but they
>>> will not be offloaded to hardware and are only visible as kernel dp entries.
>>>
>>> I guess this might not be a real problem. However, people who upgrade OVS
>>> with tc offload might get confused why they now get these drop rules in the
>>> kernel dp. To solve this, we could simply skip adding them if hardware
>>> offload is enabled. So the behavior stays the same. Something like this
>>> (not tested, and we probably need to add dpif_is_netlink()):
>>>
>>>
>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>>> index 29f4daa63..bc39e45bd 100644
>>> --- a/ofproto/ofproto-dpif-xlate.c
>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>> @@ -34,12 +34,14 @@
>>> #include "csum.h"
>>> #include "dp-packet.h"
>>> #include "dpif.h"
>>> +#include "dpif-netdev.h"
>>> #include "in-band.h"
>>> #include "lacp.h"
>>> #include "learn.h"
>>> #include "mac-learning.h"
>>> #include "mcast-snooping.h"
>>> #include "multipath.h"
>>> +#include "netdev-offload.h"
>>> #include "netdev-vport.h"
>>> #include "netlink.h"
>>> #include "nx-match.h"
>>> @@ -8199,7 +8201,8 @@ exit:
>>>
>>> /* Install drop action if datapath supports explicit drop action. */
>>> if (xin->odp_actions && !xin->odp_actions->size &&
>>> -ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
>>> +ovs_explicit_drop_action_supported(ctx.xbridge->ofproto) &&
>>> +/* Do not add drop actions if TC hardware offload is enabled, as
>>> + * as TC does not support offloading the drop action. */
>>> +(!netdev_is_flow_api_enabled() || dpif_is_netdev(xbridge->dpif))) {
>>
>> I'd argue that the check should not be here. We should not set the
>> baker.rt_support flag in the first place instead.
>
> Yes, that was my initial thought also, but what if hw offload gets enabled
> later? We might need some re-evaluation at that stage. I know the docs say
> you have to restart ovs when you enable HW offload, but that is not always
> happening in practice.
But if we keep it this way we'll have misleading information that it is
supported in the log, while it will not be used in practice.
>
>>> put_drop_action(xin->odp_actions, ctx.error);
>>> }
>>>
>>> I do not see a way for TC to support this as is in the kernel. We probably
>>> need some new kind of drop reason supported TC action.
>>>
>>> I know other people are exploring to see if this explicit drop action can
>>> be used for other, more user-level things. If this is true this TC part
>>> needs some proper fixing or we end up with rules not being offloaded.
>>>
>>> Added some more people, who might have some opinions on TC offload / ideas
>>> on how to add SKB_DROP_REASON support to TC.
>>>
>>> Cheers,
>>>
>>>
>>> Eelco
>>>
Signed-off-by: Eric Garver
---
include/linux/openvswitch.h | 2 +-
lib/dpif.c | 6 --
lib/dpif.h | 1 -
ofproto/ofproto-dpif.c | 34 --
4 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index a265e05ad253..fce6456f47c8 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1086,11 +1086,11 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
+ OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
#ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
- OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
#endif
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
diff --git a/lib/dpif.c b/lib/dpif.c
index d328bf288de0..dc02ec912693 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
return dpif_is_netdev(dpif);
}
-bool
-dpif_supports_explicit_drop_action(const struct dpif *dpif)
-{
-return dpif_is_netdev(dpif);
-}
-
bool
Re: [ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP
On 7 Jul 2023, at 13:03, Ilya Maximets wrote:
> On 7/6/23 11:30, Eelco Chaudron wrote:
>>
>>
>> On 30 Jun 2023, at 21:05, Eric Garver wrote:
>>
>> Hi Eric,
>>
>> I guess some description here would be good. Maybe add some info that this
>> is added due to kernel support being added.
>>
>>
>> In general, the patch looks good, and I would ack it. However, there is a
>> potential issue with TC hardware offload. If the patch gets applied as is,
>> and the kernel fix gets in, the additional DROP actions get added, but they
>> will not be offloaded to hardware and are only visible as kernel dp entries.
>>
>> I guess this might not be a real problem. However, people who upgrade OVS
>> with tc offload might get confused why they now get these drop rules in the
>> kernel dp. To solve this, we could simply skip adding them if hardware
>> offload is enabled. So the behavior stays the same. Something like this (not
>> tested, and we probably need to add dpif_is_netlink()):
>>
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 29f4daa63..bc39e45bd 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -34,12 +34,14 @@
>> #include "csum.h"
>> #include "dp-packet.h"
>> #include "dpif.h"
>> +#include "dpif-netdev.h"
>> #include "in-band.h"
>> #include "lacp.h"
>> #include "learn.h"
>> #include "mac-learning.h"
>> #include "mcast-snooping.h"
>> #include "multipath.h"
>> +#include "netdev-offload.h"
>> #include "netdev-vport.h"
>> #include "netlink.h"
>> #include "nx-match.h"
>> @@ -8199,7 +8201,8 @@ exit:
>>
>> /* Install drop action if datapath supports explicit drop action. */
>> if (xin->odp_actions && !xin->odp_actions->size &&
>> -ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
>> +ovs_explicit_drop_action_supported(ctx.xbridge->ofproto) &&
>> +/* Do not add drop actions if TC hardware offload is enabled, as
>> + * as TC does not support offloading the drop action. */
>> +(!netdev_is_flow_api_enabled() || dpif_is_netdev(xbridge->dpif))) {
>
> I'd argue that the check should not be here. We should not set the
> baker.rt_support flag in the first place instead.
Yes, that was my initial thought also, but what if hw offload gets enabled
later? We might need some re-evaluation at that stage. I know the docs say you
have to restart ovs when you enable HW offload, but that is not always
happening in practice.
>> put_drop_action(xin->odp_actions, ctx.error);
>> }
>>
>> I do not see a way for TC to support this as is in the kernel. We probably
>> need some new kind of drop reason supported TC action.
>>
>> I know other people are exploring to see if this explicit drop action can be
>> used for other, more user-level things. If this is true this TC part needs
>> some proper fixing or we end up with rules not being offloaded.
>>
>> Added some more people, who might have some opinions on TC offload / ideas
>> on how to add SKB_DROP_REASON support to TC.
>>
>> Cheers,
>>
>>
>> Eelco
>>
>>> Signed-off-by: Eric Garver
>>> ---
>>> include/linux/openvswitch.h | 2 +-
>>> lib/dpif.c | 6 --
>>> lib/dpif.h | 1 -
>>> ofproto/ofproto-dpif.c | 34 --
>>> 4 files changed, 33 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>>> index a265e05ad253..fce6456f47c8 100644
>>> --- a/include/linux/openvswitch.h
>>> +++ b/include/linux/openvswitch.h
>>> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
>>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>>> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>>
>>> #ifndef __KERNEL__
>>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
>>> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>>> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
>>> #endif
>>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>> index d328bf288de0..dc02ec912693 100644
>>> --- a/lib/dpif.c
>>> +++ b/lib/dpif.c
>>> @@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>>> return dpif_is_netdev(dpif);
>>> }
>>>
>>> -bool
>>> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
>>> -{
>>> -return dpif_is_netdev(dpif);
>>> -}
>>> -
>>> bool
>>> dpif_supports_lb_output_action(const struct dpif *dpif)
>>> {
>>> diff --git a/lib/dpif.h b/lib/dpif.h
>>> index 129cbf6a1d5f..fc1719f88b4f 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -938,7 +938,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif,
>>> odp_port_t port_no,
>>>
>>>
Re: [ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP
On 7/6/23 11:30, Eelco Chaudron wrote:
>
>
> On 30 Jun 2023, at 21:05, Eric Garver wrote:
>
> Hi Eric,
>
> I guess some description here would be good. Maybe add some info that this is
> added due to kernel support being added.
>
>
> In general, the patch looks good, and I would ack it. However, there is a
> potential issue with TC hardware offload. If the patch gets applied as is,
> and the kernel fix gets in, the additional DROP actions get added, but they
> will not be offloaded to hardware and are only visible as kernel dp entries.
>
> I guess this might not be a real problem. However, people who upgrade OVS
> with tc offload might get confused why they now get these drop rules in the
> kernel dp. To solve this, we could simply skip adding them if hardware
> offload is enabled. So the behavior stays the same. Something like this (not
> tested, and we probably need to add dpif_is_netlink()):
>
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 29f4daa63..bc39e45bd 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -34,12 +34,14 @@
> #include "csum.h"
> #include "dp-packet.h"
> #include "dpif.h"
> +#include "dpif-netdev.h"
> #include "in-band.h"
> #include "lacp.h"
> #include "learn.h"
> #include "mac-learning.h"
> #include "mcast-snooping.h"
> #include "multipath.h"
> +#include "netdev-offload.h"
> #include "netdev-vport.h"
> #include "netlink.h"
> #include "nx-match.h"
> @@ -8199,7 +8201,8 @@ exit:
>
> /* Install drop action if datapath supports explicit drop action. */
> if (xin->odp_actions && !xin->odp_actions->size &&
> -ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
> +ovs_explicit_drop_action_supported(ctx.xbridge->ofproto) &&
> +/* Do not add drop actions if TC hardware offload is enabled, as
> + * as TC does not support offloading the drop action. */
> +(!netdev_is_flow_api_enabled() || dpif_is_netdev(xbridge->dpif))) {
I'd argue that the check should not be here. We should not set the
baker.rt_support flag in the first place instead.
> put_drop_action(xin->odp_actions, ctx.error);
> }
>
> I do not see a way for TC to support this as is in the kernel. We probably
> need some new kind of drop reason supported TC action.
>
> I know other people are exploring to see if this explicit drop action can be
> used for other, more user-level things. If this is true this TC part needs
> some proper fixing or we end up with rules not being offloaded.
>
> Added some more people, who might have some opinions on TC offload / ideas on
> how to add SKB_DROP_REASON support to TC.
>
> Cheers,
>
>
> Eelco
>
>> Signed-off-by: Eric Garver
>> ---
>> include/linux/openvswitch.h | 2 +-
>> lib/dpif.c | 6 --
>> lib/dpif.h | 1 -
>> ofproto/ofproto-dpif.c | 34 --
>> 4 files changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index a265e05ad253..fce6456f47c8 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
>> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
>> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
>> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
>> +OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>>
>> #ifndef __KERNEL__
>> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
>> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
>> -OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
>> #endif
>> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index d328bf288de0..dc02ec912693 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
>> return dpif_is_netdev(dpif);
>> }
>>
>> -bool
>> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
>> -{
>> -return dpif_is_netdev(dpif);
>> -}
>> -
>> bool
>> dpif_supports_lb_output_action(const struct dpif *dpif)
>> {
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index 129cbf6a1d5f..fc1719f88b4f 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -938,7 +938,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif,
>> odp_port_t port_no,
>>
>> char *dpif_get_dp_version(const struct dpif *);
>> bool dpif_supports_tnl_push_pop(const struct dpif *);
>> -bool dpif_supports_explicit_drop_action(const struct dpif *);
>> bool dpif_synced_dp_layers(struct dpif *);
>>
>> /* Log functions. */
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index fad7342b0b02..c490a3c1da48 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++
Re: [ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP
On 30 Jun 2023, at 21:05, Eric Garver wrote:
Hi Eric,
I guess some description here would be good. Maybe add some info that this is
added due to kernel support being added.
In general, the patch looks good, and I would ack it. However, there is a
potential issue with TC hardware offload. If the patch gets applied as is, and
the kernel fix gets in, the additional DROP actions get added, but they will
not be offloaded to hardware and are only visible as kernel dp entries.
I guess this might not be a real problem. However, people who upgrade OVS with
tc offload might get confused why they now get these drop rules in the kernel
dp. To solve this, we could simply skip adding them if hardware offload is
enabled. So the behavior stays the same. Something like this (not tested, and
we probably need to add dpif_is_netlink()):
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 29f4daa63..bc39e45bd 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -34,12 +34,14 @@
#include "csum.h"
#include "dp-packet.h"
#include "dpif.h"
+#include "dpif-netdev.h"
#include "in-band.h"
#include "lacp.h"
#include "learn.h"
#include "mac-learning.h"
#include "mcast-snooping.h"
#include "multipath.h"
+#include "netdev-offload.h"
#include "netdev-vport.h"
#include "netlink.h"
#include "nx-match.h"
@@ -8199,7 +8201,8 @@ exit:
/* Install drop action if datapath supports explicit drop action. */
if (xin->odp_actions && !xin->odp_actions->size &&
-ovs_explicit_drop_action_supported(ctx.xbridge->ofproto)) {
+ovs_explicit_drop_action_supported(ctx.xbridge->ofproto) &&
+/* Do not add drop actions if TC hardware offload is enabled, as
+ * as TC does not support offloading the drop action. */
+(!netdev_is_flow_api_enabled() || dpif_is_netdev(xbridge->dpif))) {
put_drop_action(xin->odp_actions, ctx.error);
}
I do not see a way for TC to support this as is in the kernel. We probably need
some new kind of drop reason supported TC action.
I know other people are exploring to see if this explicit drop action can be
used for other, more user-level things. If this is true this TC part needs some
proper fixing or we end up with rules not being offloaded.
Added some more people, who might have some opinions on TC offload / ideas on
how to add SKB_DROP_REASON support to TC.
Cheers,
Eelco
> Signed-off-by: Eric Garver
> ---
> include/linux/openvswitch.h | 2 +-
> lib/dpif.c | 6 --
> lib/dpif.h | 1 -
> ofproto/ofproto-dpif.c | 34 --
> 4 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index a265e05ad253..fce6456f47c8 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -1086,11 +1086,11 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
> OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
> + OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
>
> #ifndef __KERNEL__
> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
> - OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
> OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
> #endif
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
> diff --git a/lib/dpif.c b/lib/dpif.c
> index d328bf288de0..dc02ec912693 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
> return dpif_is_netdev(dpif);
> }
>
> -bool
> -dpif_supports_explicit_drop_action(const struct dpif *dpif)
> -{
> -return dpif_is_netdev(dpif);
> -}
> -
> bool
> dpif_supports_lb_output_action(const struct dpif *dpif)
> {
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 129cbf6a1d5f..fc1719f88b4f 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -938,7 +938,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif,
> odp_port_t port_no,
>
> char *dpif_get_dp_version(const struct dpif *);
> bool dpif_supports_tnl_push_pop(const struct dpif *);
> -bool dpif_supports_explicit_drop_action(const struct dpif *);
> bool dpif_synced_dp_layers(struct dpif *);
>
> /* Log functions. */
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index fad7342b0b02..c490a3c1da48 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1375,6 +1375,37 @@ check_ct_timeout_policy(struct dpif_backer *backer)
> return !error;
> }
>
> +/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action.
> */
> +static bool
> +check_drop_action(struct dpif_backer *backer)
> +{
> +struct odputil_keybuf keybuf;
> +uint8_t
[ovs-dev] [PATCH v2 2/4] dpif: probe support for OVS_ACTION_ATTR_DROP
Signed-off-by: Eric Garver
---
include/linux/openvswitch.h | 2 +-
lib/dpif.c | 6 --
lib/dpif.h | 1 -
ofproto/ofproto-dpif.c | 34 --
4 files changed, 33 insertions(+), 10 deletions(-)
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index a265e05ad253..fce6456f47c8 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -1086,11 +1086,11 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */
+ OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
#ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
- OVS_ACTION_ATTR_DROP, /* u32 xlate_error. */
OVS_ACTION_ATTR_LB_OUTPUT, /* u32 bond-id. */
#endif
__OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
diff --git a/lib/dpif.c b/lib/dpif.c
index d328bf288de0..dc02ec912693 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1928,12 +1928,6 @@ dpif_supports_tnl_push_pop(const struct dpif *dpif)
return dpif_is_netdev(dpif);
}
-bool
-dpif_supports_explicit_drop_action(const struct dpif *dpif)
-{
-return dpif_is_netdev(dpif);
-}
-
bool
dpif_supports_lb_output_action(const struct dpif *dpif)
{
diff --git a/lib/dpif.h b/lib/dpif.h
index 129cbf6a1d5f..fc1719f88b4f 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -938,7 +938,6 @@ int dpif_get_pmds_for_port(const struct dpif * dpif,
odp_port_t port_no,
char *dpif_get_dp_version(const struct dpif *);
bool dpif_supports_tnl_push_pop(const struct dpif *);
-bool dpif_supports_explicit_drop_action(const struct dpif *);
bool dpif_synced_dp_layers(struct dpif *);
/* Log functions. */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index fad7342b0b02..c490a3c1da48 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1375,6 +1375,37 @@ check_ct_timeout_policy(struct dpif_backer *backer)
return !error;
}
+/* Tests whether backer's datapath supports the OVS_ACTION_ATTR_DROP action. */
+static bool
+check_drop_action(struct dpif_backer *backer)
+{
+struct odputil_keybuf keybuf;
+uint8_t actbuf[NL_A_U32_SIZE];
+struct ofpbuf actions;
+struct ofpbuf key;
+struct flow flow;
+bool supported;
+
+struct odp_flow_key_parms odp_parms = {
+.flow = ,
+.probe = true,
+};
+
+memset(, 0, sizeof flow);
+ofpbuf_use_stack(, , sizeof keybuf);
+odp_flow_key_from_flow(_parms, );
+
+ofpbuf_use_stack(, , sizeof actbuf);
+nl_msg_put_u32(, OVS_ACTION_ATTR_DROP, XLATE_OK);
+
+supported = dpif_probe_feature(backer->dpif, "drop", , , NULL);
+
+VLOG_INFO("%s: Datapath %s drop action",
+ dpif_name(backer->dpif), (supported) ? "supports"
+ : "does not support");
+return supported;
+}
+
/* Tests whether 'backer''s datapath supports the all-zero SNAT case. */
static bool
dpif_supports_ct_zero_snat(struct dpif_backer *backer)
@@ -1627,8 +1658,7 @@ check_support(struct dpif_backer *backer)
backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
backer->rt_support.check_pkt_len = check_check_pkt_len(backer);
backer->rt_support.ct_timeout = check_ct_timeout_policy(backer);
-backer->rt_support.explicit_drop_action =
-dpif_supports_explicit_drop_action(backer->dpif);
+backer->rt_support.explicit_drop_action = check_drop_action(backer);
backer->rt_support.lb_output_action =
dpif_supports_lb_output_action(backer->dpif);
backer->rt_support.ct_zero_snat = dpif_supports_ct_zero_snat(backer);
--
2.39.0
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
