[ovs-dev] [PATCH RFC v2] ofproto: RFC extended statistics patch.

Ben Pfaff blp at ovn.org
Mon Nov 30 01:05:03 UTC 2015


On Mon, Nov 23, 2015 at 09:10:40AM +0000, mweglicx wrote:
> Implementation of new statistics extension:
> - new counters definition based on RFC2819,
> - new statistics are retrieved using experimenter code and
>   are printed as a result to ofctl dump-ports,
> - new statistics are printed to output via ofctl only if those
>   are present in reply message,
> - new file header created: include/openflow/intel-ext.h which
>   contains new statistics definition,
> - new extended statistics calculation is implemented only for
>   dpdk-vhost-enabled ports.
> 
> Please note that this is just feature proposal:
> - experimenter code is hardcoded in ofp-util.c, it will be
>   changed when VENDOR_ID for intel will be defined,
> - final patch will include also more counters based on
>   RFC2863, RFC3635 and RFC2819.
> 
> v2:
> - protocol extension in ofproto has been removed, existing
>   mechanism with experimenter codes has been used,
> - additional option in ofctl dump-ports-ext has been removed,
>   new counters are retrieved via dump-ports.
> 
> Signed-off-by: Michal Weglicki <michalx.weglicki at intel.com>
> Signed-off-by: Timo Puha <timox.puha at intel.com>

This version seems quite reasonable.

I'm confused by the tentative choice of vendor ID, because it doesn't
seem to be an Ethernet OUI for Intel.  Rather, it seems to belong to:

00-15-1D   (hex)		M2I CORPORATION
00151D     (base 16)		M2I CORPORATION
				Kyonggi Venture Anyang technical center, 13th Floor, 572-5, Anyang 8-Dong, Manan-Gu
				Anyang-Shi  Kyonggi-Do  430-731
				KR

It seems like you could just choose any Ethernet OUI for Intel and put
it in the openflow header; not sure why you're putting it in ofp-util.c
instead.

There is a lot of code duplication here among all of the netdev
implementations, to initialize the added statistics to "unknown"
values.  It would be better to avoid the duplication.  One way would be
to add a global helper function; another would be for netdev_get_stats()
to initialize the whole structure to 0xff before passing it to the
netdev implementation.

I'll look forward to the next revision.



More information about the dev mailing list