| Message ID | 1564694047-4859-1-git-send-email-pkusunyifeng@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] openvswitch: Print error when ovs_execute_actions() fails | expand |
On 8/1/2019 2:14 PM, Yifeng Sun wrote: > Currently in function ovs_dp_process_packet(), return values of > ovs_execute_actions() are silently discarded. This patch prints out > an error message when error happens so as to provide helpful hints > for debugging. > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > --- > net/openvswitch/datapath.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index 892287d..603c533 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -222,6 +222,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > struct dp_stats_percpu *stats; > u64 *stats_counter; > u32 n_mask_hit; > + int error; > > stats = this_cpu_ptr(dp->stats_percpu); > > @@ -229,7 +230,6 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > flow = ovs_flow_tbl_lookup_stats(&dp->table, key, &n_mask_hit); > if (unlikely(!flow)) { > struct dp_upcall_info upcall; > - int error; > > memset(&upcall, 0, sizeof(upcall)); > upcall.cmd = OVS_PACKET_CMD_MISS; > @@ -246,7 +246,10 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > > ovs_flow_stats_update(flow, key->tp.flags, skb); > sf_acts = rcu_dereference(flow->sf_acts); > - ovs_execute_actions(dp, skb, sf_acts, key); > + error = ovs_execute_actions(dp, skb, sf_acts, key); > + if (unlikely(error)) > + net_err_ratelimited("ovs: action execution error on datapath %s: %d\n", > + ovs_dp_name(dp), error); > > stats_counter = &stats->n_hit; > Thanks Yifeng. Reviewed-by: Greg Rose <gvrose8192@gmail.com>
On Thu, Aug 1, 2019 at 2:14 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > Currently in function ovs_dp_process_packet(), return values of > ovs_execute_actions() are silently discarded. This patch prints out > an error message when error happens so as to provide helpful hints > for debugging. > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > --- > net/openvswitch/datapath.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index 892287d..603c533 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -222,6 +222,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > struct dp_stats_percpu *stats; > u64 *stats_counter; > u32 n_mask_hit; > + int error; > > stats = this_cpu_ptr(dp->stats_percpu); > > @@ -229,7 +230,6 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > flow = ovs_flow_tbl_lookup_stats(&dp->table, key, &n_mask_hit); > if (unlikely(!flow)) { > struct dp_upcall_info upcall; > - int error; > > memset(&upcall, 0, sizeof(upcall)); > upcall.cmd = OVS_PACKET_CMD_MISS; > @@ -246,7 +246,10 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > > ovs_flow_stats_update(flow, key->tp.flags, skb); > sf_acts = rcu_dereference(flow->sf_acts); > - ovs_execute_actions(dp, skb, sf_acts, key); > + error = ovs_execute_actions(dp, skb, sf_acts, key); > + if (unlikely(error)) > + net_err_ratelimited("ovs: action execution error on datapath %s: %d\n", > + ovs_dp_name(dp), error); > I would rather add error counter for better visibility. If you want to use current approach, can you use net_dbg_ratelimited() since you want to use this for debugging purpose? Thanks.
Yes, this fix is mainly for debugging purposes. If packets are blackholed because of errors from ovs_execute_actions(), we can got more helpful information. Thanks Pravin for the review, I will come up with a new version. Yifeng On Sat, Aug 3, 2019 at 4:00 PM Pravin Shelar <pshelar@ovn.org> wrote: > > On Thu, Aug 1, 2019 at 2:14 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > > > Currently in function ovs_dp_process_packet(), return values of > > ovs_execute_actions() are silently discarded. This patch prints out > > an error message when error happens so as to provide helpful hints > > for debugging. > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> > > --- > > net/openvswitch/datapath.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > > index 892287d..603c533 100644 > > --- a/net/openvswitch/datapath.c > > +++ b/net/openvswitch/datapath.c > > @@ -222,6 +222,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > > struct dp_stats_percpu *stats; > > u64 *stats_counter; > > u32 n_mask_hit; > > + int error; > > > > stats = this_cpu_ptr(dp->stats_percpu); > > > > @@ -229,7 +230,6 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > > flow = ovs_flow_tbl_lookup_stats(&dp->table, key, &n_mask_hit); > > if (unlikely(!flow)) { > > struct dp_upcall_info upcall; > > - int error; > > > > memset(&upcall, 0, sizeof(upcall)); > > upcall.cmd = OVS_PACKET_CMD_MISS; > > @@ -246,7 +246,10 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) > > > > ovs_flow_stats_update(flow, key->tp.flags, skb); > > sf_acts = rcu_dereference(flow->sf_acts); > > - ovs_execute_actions(dp, skb, sf_acts, key); > > + error = ovs_execute_actions(dp, skb, sf_acts, key); > > + if (unlikely(error)) > > + net_err_ratelimited("ovs: action execution error on datapath %s: %d\n", > > + ovs_dp_name(dp), error); > > > > I would rather add error counter for better visibility. > If you want to use current approach, can you use net_dbg_ratelimited() > since you want to use this for debugging purpose? > > Thanks.
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c index 892287d..603c533 100644 --- a/net/openvswitch/datapath.c +++ b/net/openvswitch/datapath.c @@ -222,6 +222,7 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) struct dp_stats_percpu *stats; u64 *stats_counter; u32 n_mask_hit; + int error; stats = this_cpu_ptr(dp->stats_percpu); @@ -229,7 +230,6 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) flow = ovs_flow_tbl_lookup_stats(&dp->table, key, &n_mask_hit); if (unlikely(!flow)) { struct dp_upcall_info upcall; - int error; memset(&upcall, 0, sizeof(upcall)); upcall.cmd = OVS_PACKET_CMD_MISS; @@ -246,7 +246,10 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key) ovs_flow_stats_update(flow, key->tp.flags, skb); sf_acts = rcu_dereference(flow->sf_acts); - ovs_execute_actions(dp, skb, sf_acts, key); + error = ovs_execute_actions(dp, skb, sf_acts, key); + if (unlikely(error)) + net_err_ratelimited("ovs: action execution error on datapath %s: %d\n", + ovs_dp_name(dp), error); stats_counter = &stats->n_hit;
Currently in function ovs_dp_process_packet(), return values of ovs_execute_actions() are silently discarded. This patch prints out an error message when error happens so as to provide helpful hints for debugging. Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com> --- net/openvswitch/datapath.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)