>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 <wangchuan...@inspur.com>> >> >> 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.
Yes, 'UPCALL ...' approach better, may be 'UPCALL packets:X errors:Y' is better, after all, failed/dropped/errors means the kernel datapath stopped upcall to userspce when it find errors in the info of packet. > >> >> 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? I already tested it, if datapath didn't support the feature, it will print '?'. > >> Anyone any thoughts!? > >This patch is missing the stats propagation to the database in the >iface_refresh_stats(). That should be added. Ok! > >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. Userspace datapath is not count in dpctl command, we can do not include this patch ? Any ideas??? Best regards, wangchuanlei.
_______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
