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 <[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. > > 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. >>> >>> 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. >> >>> 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？ 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? > Best regards, wangchuanlei. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
