diff mbox series

[ovs-dev] lacp: warn transmit failure of lacp pdu

Message ID 20191021140436.24696-1-gmuthukr@redhat.com
State Accepted
Headers show
Series [ovs-dev] lacp: warn transmit failure of lacp pdu | expand

Commit Message

Gowrishankar Muthukrishnan Oct. 21, 2019, 2:04 p.m. UTC
It might be difficult to trace whether LACP PDU tx (as in
response) was successful when the pdu was not transmitted by
egress slave for various reasons (including resource contention
within NIC) and only way to trace its fate is by looking at
ofproto->stats.tx_[packets/bytes] and slave port stats.

Adding a warning when there is tx failure could help user
debug at the root of this problem.

Signed-off-by: Gowrishankar Muthukrishnan <gmuthukr@redhat.com>
---
 ofproto/ofproto-dpif.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Ben Pfaff Oct. 24, 2019, 8:31 p.m. UTC | #1
On Mon, Oct 21, 2019 at 07:34:36PM +0530, Gowrishankar Muthukrishnan wrote:
> It might be difficult to trace whether LACP PDU tx (as in
> response) was successful when the pdu was not transmitted by
> egress slave for various reasons (including resource contention
> within NIC) and only way to trace its fate is by looking at
> ofproto->stats.tx_[packets/bytes] and slave port stats.
> 
> Adding a warning when there is tx failure could help user
> debug at the root of this problem.
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukr@redhat.com>

This logic seems like it's probably true for all users of
ofproto_dpif_send_packet(), not just LACP PDU transmission.  Would it be
a good idea to simply make ofproto_dpif_send_packet() log a warning when
tx fails?
Gowrishankar Muthukrishnan Oct. 29, 2019, 8:51 a.m. UTC | #2
Hi Ben,
Idea is to log lacp specific tx failure. A one step further as in
ofproto_dpif_send_packet has to make the debug info more generic, even
though it would cater all of its calling functions.
For example, bundle_send_learning_packets reports such tx failures for
gratuitous packets. I think, we need to handle such warning at the calling
place itself to debug quickly.

Thanks,
Gowrishankar

On Fri, Oct 25, 2019 at 2:47 AM Ben Pfaff <blp@ovn.org> wrote:

> On Mon, Oct 21, 2019 at 07:34:36PM +0530, Gowrishankar Muthukrishnan wrote:
> > It might be difficult to trace whether LACP PDU tx (as in
> > response) was successful when the pdu was not transmitted by
> > egress slave for various reasons (including resource contention
> > within NIC) and only way to trace its fate is by looking at
> > ofproto->stats.tx_[packets/bytes] and slave port stats.
> >
> > Adding a warning when there is tx failure could help user
> > debug at the root of this problem.
> >
> > Signed-off-by: Gowrishankar Muthukrishnan <gmuthukr@redhat.com>
>
> This logic seems like it's probably true for all users of
> ofproto_dpif_send_packet(), not just LACP PDU transmission.  Would it be
> a good idea to simply make ofproto_dpif_send_packet() log a warning when
> tx fails?
>
Ben Pfaff Oct. 30, 2019, 5:51 p.m. UTC | #3
On Mon, Oct 21, 2019 at 07:34:36PM +0530, Gowrishankar Muthukrishnan wrote:
> It might be difficult to trace whether LACP PDU tx (as in
> response) was successful when the pdu was not transmitted by
> egress slave for various reasons (including resource contention
> within NIC) and only way to trace its fate is by looking at
> ofproto->stats.tx_[packets/bytes] and slave port stats.
> 
> Adding a warning when there is tx failure could help user
> debug at the root of this problem.
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gmuthukr@redhat.com>

I applied this to master, thanks!
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 496a16c8a..4af771585 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3405,7 +3405,11 @@  send_pdu_cb(void *port_, const void *pdu, size_t pdu_size)
                                  pdu_size);
         memcpy(packet_pdu, pdu, pdu_size);
 
-        ofproto_dpif_send_packet(port, false, &packet);
+        error = ofproto_dpif_send_packet(port, false, &packet);
+        if (error) {
+            VLOG_WARN_RL(&rl, "port %s: cannot transmit LACP PDU (%s).",
+                         port->bundle->name, ovs_strerror(error));
+        }
         dp_packet_uninit(&packet);
     } else {
         static struct vlog_rate_limit rll = VLOG_RATE_LIMIT_INIT(1, 10);