On 1/6/23 11:12, Eelco Chaudron wrote: > > > On 6 Jan 2023, at 10:49, wangchuanlei wrote: > >>> 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. > > Ok, so let’s do the UPCALL, and I also think error would fit best.
+1 'UPCALL packets:X errors:Y' indeed looks better. One other thought here is that 'success' and 'failure' are meaningful from the kernel side of things, but a bit ambiguous from the OVS point of view, because upcall may fail somewhere in the flow translation in userspace and packet will be dropped, but the counter will say 'upcall success', which is a bit misleading. 'errors' from that point of view is a bit better term. Not ideal, but better than other options. > >>>> >>>> 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 >> '?'. > > ACK, did test this also. OK, good. Thanks! > >>> >>>> 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 ? As I mentioned above, it might be hard to track upcalls this way for userpsace datapath. The reason is that upcalls are happening on the datapath level (dpif-netdev), but statistics are queried from the netdev level. Getting upcall stats from the dpif-netdev down to netdev stats might be tricky from both the logical and performance point of view. So, I think, it's OK to always report question marks for ports in userspace datapath for now. No extra changes required. >> >> Any ideas？ > > I guess you can just add them in iface_refresh_stats() ‘#define IFACE_STATS’ > definition, and they will be included if not set the UINT64_MAX. Or do I miss > anything? Yes, we only need to update the IFACE_STATS definition. We should probably alter the names of the counters though the same way we agreed to do for the dpctl output, i.e. report 'upcall_success' as 'upcall_packets' and 'upcall_fail' as 'upcall_errors'. Doe that make sense? > >> Best regards, wangchuanlei. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
