| Message ID | 20190923141510.10303-3-fbl@sysclose.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/3] support for large mpool. | expand |
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
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()?
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 > } > }
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
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
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
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 } }
Signed-off-by: Flavio Leitner <fbl@sysclose.org> --- lib/netdev-dpdk.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)