[ovs-dev,3/3] debug: print mbuf extra info.
diff mbox series

Message ID 20190923141510.10303-3-fbl@sysclose.org
State New
Headers show
Series
  • [ovs-dev,1/3] support for large mpool.
Related show

Commit Message

Flavio Leitner Sept. 23, 2019, 2:15 p.m. UTC
Signed-off-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/netdev-dpdk.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

0-day Robot Sept. 23, 2019, 3:05 p.m. UTC | #1
Bleep bloop.  Greetings Flavio Leitner, 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:
ERROR: Inappropriate bracing around statement
#23 FILE: lib/netdev-dpdk.c:2267:
        if (nb_segs == 1)

ERROR: Inappropriate bracing around statement
#27 FILE: lib/netdev-dpdk.c:2271:
        else

Lines checked: 42, Warnings: 0, Errors: 2


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Ben Pfaff Sept. 26, 2019, 2:49 p.m. UTC | #2
On Thu, Sep 26, 2019 at 11:59:14AM -0400, Aaron Conole wrote:
> Flavio Leitner <fbl@sysclose.org> writes:
> 
> > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > ---
> >  lib/netdev-dpdk.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > index cfbd9a9e5..7965bf57a 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -2294,6 +2294,24 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
> >          }
> >  
> >          stats->rx_bytes += packet_size;
> > +#if 0
> 
> I've never liked bare '#if 0' in the code.
> 
> Can you make this something like:
> 
>   #ifndef NDEBUG
> 
> This means for all debug builds we would get this tracing
> infrastructure.  Maybe even make a special configuration option?

It's so much better if we can avoid #ifdefs.  Can we just use VLOG_DBG()
or its related VLOG_IS_DBG_ENABLED(), VLOG_DBG_RL(), VLOG_DROP_DBG()?
Aaron Conole Sept. 26, 2019, 3:59 p.m. UTC | #3
Flavio Leitner <fbl@sysclose.org> writes:

> Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  lib/netdev-dpdk.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index cfbd9a9e5..7965bf57a 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2294,6 +2294,24 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
>          }
>  
>          stats->rx_bytes += packet_size;
> +#if 0

I've never liked bare '#if 0' in the code.

Can you make this something like:

  #ifndef NDEBUG

This means for all debug builds we would get this tracing
infrastructure.  Maybe even make a special configuration option?

> +        /* Debug/Tracing */
> +        struct rte_mbuf *mbuf = (struct rte_mbuf *)packet;
> +        int nb_segs = mbuf->nb_segs;
> +        if (nb_segs == 1)
> +                VLOG_INFO("single (root) size = %d, nb_segs = %d, max = %d",
> +                          mbuf->pkt_len, nb_segs,
> +                          rte_pktmbuf_data_room_size(mbuf->pool));
> +        else
> +                VLOG_INFO("chained (root) size = %d, nb_segs = %d, max = %d",
> +                          mbuf->pkt_len, nb_segs,
> +                          rte_pktmbuf_data_room_size(mbuf->pool));
> +
> +        for (int j = 1; j < nb_segs; j++) {
> +                mbuf = mbuf->next;
> +                VLOG_INFO("chained packet (%d) size = %d", j, mbuf->data_len);
> +        }
> +#endif
>      }
>  }
Flavio Leitner Sept. 27, 2019, 1:05 p.m. UTC | #4
On Thu, Sep 26, 2019 at 07:49:28AM -0700, Ben Pfaff wrote:
> On Thu, Sep 26, 2019 at 11:59:14AM -0400, Aaron Conole wrote:
> > Flavio Leitner <fbl@sysclose.org> writes:
> > 
> > > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > > ---
> > >  lib/netdev-dpdk.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > >
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > > index cfbd9a9e5..7965bf57a 100644
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -2294,6 +2294,24 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
> > >          }
> > >  
> > >          stats->rx_bytes += packet_size;
> > > +#if 0
> > 
> > I've never liked bare '#if 0' in the code.
> > 
> > Can you make this something like:
> > 
> >   #ifndef NDEBUG
> > 
> > This means for all debug builds we would get this tracing
> > infrastructure.  Maybe even make a special configuration option?
> 
> It's so much better if we can avoid #ifdefs.  Can we just use VLOG_DBG()
> or its related VLOG_IS_DBG_ENABLED(), VLOG_DBG_RL(), VLOG_DROP_DBG()?

My bad, I missed to change the subject prefix to PoC or RFC.
Please do NOT apply the patchset, there are bugs and it is complete.
They are just development patches to show the alternative idea.

fbl
Flavio Leitner Sept. 27, 2019, 1:18 p.m. UTC | #5
On Fri, Sep 27, 2019 at 10:05:53AM -0300, Flavio Leitner wrote:
> On Thu, Sep 26, 2019 at 07:49:28AM -0700, Ben Pfaff wrote:
> > On Thu, Sep 26, 2019 at 11:59:14AM -0400, Aaron Conole wrote:
> > > Flavio Leitner <fbl@sysclose.org> writes:
> > > 
> > > > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
> > > > ---
> > > >  lib/netdev-dpdk.c | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > >
> > > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> > > > index cfbd9a9e5..7965bf57a 100644
> > > > --- a/lib/netdev-dpdk.c
> > > > +++ b/lib/netdev-dpdk.c
> > > > @@ -2294,6 +2294,24 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
> > > >          }
> > > >  
> > > >          stats->rx_bytes += packet_size;
> > > > +#if 0
> > > 
> > > I've never liked bare '#if 0' in the code.
> > > 
> > > Can you make this something like:
> > > 
> > >   #ifndef NDEBUG
> > > 
> > > This means for all debug builds we would get this tracing
> > > infrastructure.  Maybe even make a special configuration option?
> > 
> > It's so much better if we can avoid #ifdefs.  Can we just use VLOG_DBG()
> > or its related VLOG_IS_DBG_ENABLED(), VLOG_DBG_RL(), VLOG_DROP_DBG()?
> 
> My bad, I missed to change the subject prefix to PoC or RFC.
> Please do NOT apply the patchset, there are bugs and it is complete.

*incomplete
Aaron Conole Sept. 27, 2019, 8:18 p.m. UTC | #6
Flavio Leitner <fbl@sysclose.org> writes:

> On Thu, Sep 26, 2019 at 07:49:28AM -0700, Ben Pfaff wrote:
>> On Thu, Sep 26, 2019 at 11:59:14AM -0400, Aaron Conole wrote:
>> > Flavio Leitner <fbl@sysclose.org> writes:
>> > 
>> > > Signed-off-by: Flavio Leitner <fbl@sysclose.org>
>> > > ---
>> > >  lib/netdev-dpdk.c | 18 ++++++++++++++++++
>> > >  1 file changed, 18 insertions(+)
>> > >
>> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> > > index cfbd9a9e5..7965bf57a 100644
>> > > --- a/lib/netdev-dpdk.c
>> > > +++ b/lib/netdev-dpdk.c
>> > > @@ -2294,6 +2294,24 @@ netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
>> > >          }
>> > >  
>> > >          stats->rx_bytes += packet_size;
>> > > +#if 0
>> > 
>> > I've never liked bare '#if 0' in the code.
>> > 
>> > Can you make this something like:
>> > 
>> >   #ifndef NDEBUG
>> > 
>> > This means for all debug builds we would get this tracing
>> > infrastructure.  Maybe even make a special configuration option?
>> 
>> It's so much better if we can avoid #ifdefs.  Can we just use VLOG_DBG()
>> or its related VLOG_IS_DBG_ENABLED(), VLOG_DBG_RL(), VLOG_DROP_DBG()?
>
> My bad, I missed to change the subject prefix to PoC or RFC.
> Please do NOT apply the patchset, there are bugs and it is complete.
> They are just development patches to show the alternative idea.

Dismissed without prejudice :-)

> fbl

Patch
diff mbox series

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index cfbd9a9e5..7965bf57a 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2294,6 +2294,24 @@  netdev_dpdk_vhost_update_rx_counters(struct netdev_stats *stats,
         }
 
         stats->rx_bytes += packet_size;
+#if 0
+        /* Debug/Tracing */
+        struct rte_mbuf *mbuf = (struct rte_mbuf *)packet;
+        int nb_segs = mbuf->nb_segs;
+        if (nb_segs == 1)
+                VLOG_INFO("single (root) size = %d, nb_segs = %d, max = %d",
+                          mbuf->pkt_len, nb_segs,
+                          rte_pktmbuf_data_room_size(mbuf->pool));
+        else
+                VLOG_INFO("chained (root) size = %d, nb_segs = %d, max = %d",
+                          mbuf->pkt_len, nb_segs,
+                          rte_pktmbuf_data_room_size(mbuf->pool));
+
+        for (int j = 1; j < nb_segs; j++) {
+                mbuf = mbuf->next;
+                VLOG_INFO("chained packet (%d) size = %d", j, mbuf->data_len);
+        }
+#endif
     }
 }