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 <pshe...@ovn.org> wrote:
>
> On Thu, Aug 1, 2019 at 2:14 PM Yifeng Sun <pkusunyif...@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 <pkusunyif...@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.
