On 1/5/23 16:40, Eelco Chaudron wrote: > > > On 5 Jan 2023, at 2:52, wangchuanlei wrote: > >> Add support to count upall packets, when kmod of openvswitch upcall to >> count the number of packets for upcall succeed and failed, which is a >> better way to see how many packets upcalled on every interfaces. >> >> Signed-off-by: wangchuanlei <[email protected]> > > The code works, and I see no other problems, so I’m ok to ACK it. > > However, before I do, I do think we should get some feedback on how the stats > are displayed. > > This is what it looks like now: > > port 0: my (internal) > RX packets:10 errors:0 dropped:0 overruns:0 frame:0 > upcall success:1 upcall fail:0 > TX packets:0 errors:0 dropped:0 aborted:0 carrier:0 > collisions:0 > RX bytes:1230 TX bytes:0 > > > It’s a bit confusing with the space in the name. Should we maybe separate > upcall stats completely? > Something like: > > port 0: my (internal) > RX packets:10 errors:0 dropped:0 overruns:0 frame:0 > TX packets:0 errors:0 dropped:0 aborted:0 carrier:0 > collisions:0 > RX bytes:1230 TX bytes:0 > UPCALL packets:1 failed:0 > > Also, note I used ‘packets’ and ‘failed’ to be more in line with existing > stats.
I like the 'UPCALL ...' approach better. We may even go further and have 'UPCALL packets:X dropped:Y', i.e. 'dropped' instead of 'failed'. Not sure. > > And maybe not even display it when the feature is not supported? It should be OK to print these as long as we correctly print '?' on ports that do not support that counter. Did you test that? > Anyone any thoughts!? This patch is missing the stats propagation to the database in the iface_refresh_stats(). That should be added. OTOH, a point can be made that these stats should not be part of the main netdev_stats and hence should not be reported along side of them at all. It might be better to report them via 'custom stats' mechanism, since not all the ports will have them and it might be difficult to add support for upcall counting in that form to userspace datapath, so it will always report question marks. Custom stats will be accessed via database or via OpenFlow 1.4+ version of dump-ports request. Don't have a strong opinion on this though. What do you think? Best regards, Ilya maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
