On 16.01.2018 04:51, Jan Scheurich wrote: > If implememented, this function returns the number of packets in an rx > queue of the netdev. If not implemented, it returns -1.
To be conform with other netdev functions it should return meaningful error codes. As 'rte_eth_rx_queue_count' could return different errors like -EINVAL or -ENOTSUP, 'netdev_rxq_length' itself should return -EOPNOTSUPP if not implemented. > > This function will be used in the upcoming commit for PMD performance > metrics to supervise the rx queue fill level for DPDK vhostuser ports. > > Signed-off-by: Jan Scheurich <[email protected]> > --- > lib/netdev-bsd.c | 1 + > lib/netdev-dpdk.c | 36 +++++++++++++++++++++++++++++++----- > lib/netdev-dummy.c | 1 + > lib/netdev-linux.c | 1 + > lib/netdev-provider.h | 3 +++ > lib/netdev-vport.c | 1 + > lib/netdev.c | 9 +++++++++ > lib/netdev.h | 1 + > 8 files changed, 48 insertions(+), 5 deletions(-) > > diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c > index 05974c1..8d1771e 100644 > --- a/lib/netdev-bsd.c > +++ b/lib/netdev-bsd.c > @@ -1546,6 +1546,7 @@ netdev_bsd_update_flags(struct netdev *netdev_, enum > netdev_flags off, > netdev_bsd_rxq_recv, \ > netdev_bsd_rxq_wait, \ > netdev_bsd_rxq_drain, \ > + NULL, /* rxq_length */ \ > \ > NO_OFFLOAD_API \ > } > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index ccda3fc..4200556 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1839,6 +1839,27 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq, struct > dp_packet_batch *batch) > return 0; > } > > +static int > +netdev_dpdk_vhost_rxq_length(struct netdev_rxq *rxq) > +{ > + struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev); > + int qid = rxq->queue_id; > + We must make all the checks as in rxq_recv() function before calling 'rte_vhost_rx_queue_count'. Otherwise we may crash here if device will be occasionally disconnected: int vid = netdev_dpdk_get_vid(dev); if (OVS_UNLIKELY(vid < 0 || !dev->vhost_reconfigured || !(dev->flags & NETDEV_UP))) { return -EAGAIN; } Not sure about -EAGAIN, but we need to return some negative errno. > + /* The DPDK API returns a uint32_t which often has invalid bits in the > + * upper 16-bits. Need to restrict the value uint16_t. */ > + return rte_vhost_rx_queue_count(netdev_dpdk_get_vid(dev), > + qid * VIRTIO_QNUM + VIRTIO_TXQ) > + & UINT16_MAX; > +} > + > +static int > +netdev_dpdk_rxq_length(struct netdev_rxq *rxq) > +{ > + struct netdev_rxq_dpdk *rx = netdev_rxq_dpdk_cast(rxq); > + Same here: struct netdev_dpdk *dev = netdev_dpdk_cast(rxq->netdev); if (OVS_UNLIKELY(!(dev->flags & NETDEV_UP))) { return -EAGAIN; } > + return rte_eth_rx_queue_count(rx->port_id, rxq->queue_id); > +} > + > static inline int > netdev_dpdk_qos_run(struct netdev_dpdk *dev, struct rte_mbuf **pkts, > int cnt, bool may_steal) > @@ -3580,7 +3601,7 @@ unlock: > GET_CARRIER, GET_STATS, \ > GET_CUSTOM_STATS, > \ > GET_FEATURES, GET_STATUS, \ > - RECONFIGURE, RXQ_RECV) \ > + RECONFIGURE, RXQ_RECV, RXQ_LENGTH) \ > { \ > NAME, \ > true, /* is_pmd */ \ > @@ -3649,6 +3670,7 @@ unlock: > RXQ_RECV, \ > NULL, /* rx_wait */ \ > NULL, /* rxq_drain */ \ > + RXQ_LENGTH, \ > NO_OFFLOAD_API \ > } > > @@ -3667,7 +3689,8 @@ static const struct netdev_class dpdk_class = > netdev_dpdk_get_features, > netdev_dpdk_get_status, > netdev_dpdk_reconfigure, > - netdev_dpdk_rxq_recv); > + netdev_dpdk_rxq_recv, > + netdev_dpdk_rxq_length); > > static const struct netdev_class dpdk_ring_class = > NETDEV_DPDK_CLASS( > @@ -3684,7 +3707,8 @@ static const struct netdev_class dpdk_ring_class = > netdev_dpdk_get_features, > netdev_dpdk_get_status, > netdev_dpdk_reconfigure, > - netdev_dpdk_rxq_recv); > + netdev_dpdk_rxq_recv, > + NULL); > > static const struct netdev_class dpdk_vhost_class = > NETDEV_DPDK_CLASS( > @@ -3701,7 +3725,8 @@ static const struct netdev_class dpdk_vhost_class = > NULL, > NULL, > netdev_dpdk_vhost_reconfigure, > - netdev_dpdk_vhost_rxq_recv); > + netdev_dpdk_vhost_rxq_recv, > + netdev_dpdk_vhost_rxq_length); > static const struct netdev_class dpdk_vhost_client_class = > NETDEV_DPDK_CLASS( > "dpdkvhostuserclient", > @@ -3717,7 +3742,8 @@ static const struct netdev_class > dpdk_vhost_client_class = > NULL, > NULL, > netdev_dpdk_vhost_client_reconfigure, > - netdev_dpdk_vhost_rxq_recv); > + netdev_dpdk_vhost_rxq_recv, > + netdev_dpdk_vhost_rxq_length); > > void > netdev_dpdk_register(void) > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 4246af3..7e2c0a2 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -1457,6 +1457,7 @@ netdev_dummy_update_flags(struct netdev *netdev_, > netdev_dummy_rxq_recv, \ > netdev_dummy_rxq_wait, \ > netdev_dummy_rxq_drain, \ > + NULL, /* rxq_length */ \ > \ > NO_OFFLOAD_API \ > } > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index 37143b8..8b19890 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2890,6 +2890,7 @@ netdev_linux_update_flags(struct netdev *netdev_, enum > netdev_flags off, > netdev_linux_rxq_recv, \ > netdev_linux_rxq_wait, \ > netdev_linux_rxq_drain, \ > + NULL, /* rxq_length */ \ > \ > FLOW_OFFLOAD_API \ > } > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h > index 25bd671..297644a 100644 > --- a/lib/netdev-provider.h > +++ b/lib/netdev-provider.h > @@ -801,6 +801,9 @@ struct netdev_class { > /* Discards all packets waiting to be received from 'rx'. */ > int (*rxq_drain)(struct netdev_rxq *rx); > > + /* Retrieve the number of packets present in an rx queue. */ Comment should be extended. See below. In addition we need to mark here that function is not thread-safe and could not be used while device reconfiguration. > + int (*rxq_length)(struct netdev_rxq *rx); > + > /* ## -------------------------------- ## */ > /* ## netdev flow offloading functions ## */ > /* ## -------------------------------- ## */ > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 478ed90..1e7bc96 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -944,6 +944,7 @@ netdev_vport_get_ifindex(const struct netdev *netdev_) > NULL, /* rx_recv */ \ > NULL, /* rx_wait */ \ > NULL, /* rx_drain */ \ > + NULL, /* rx_length */ \ > \ > NETDEV_FLOW_OFFLOAD_API > > diff --git a/lib/netdev.c b/lib/netdev.c > index be05dc6..063c318 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -724,6 +724,15 @@ netdev_rxq_drain(struct netdev_rxq *rx) > : 0); > } > > +/* Retrieve the number of packets present in an rx queue. */ Comment should clearly declare what kind of result should be treated as an error, and what is the result in case of success. You may use description for 'netdev_get_ifindex' as a reference. Something like: /* Returns the number of packets present in an rx queue, if successful, as a * positive number. On failure, returns a negative errno value. * * Some network devices may not implement support for this function. In such * cases this function will always return -EOPNOTSUPP. */ > +int > +netdev_rxq_length(struct netdev_rxq *rx) > +{ > + return (rx->netdev->netdev_class->rxq_length > + ? rx->netdev->netdev_class->rxq_length(rx) > + : -1); > +} > + > /* Configures the number of tx queues of 'netdev'. Returns 0 if successful, > * otherwise a positive errno value. > * > diff --git a/lib/netdev.h b/lib/netdev.h > index ff1b604..edd41b1 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -178,6 +178,7 @@ int netdev_rxq_get_queue_id(const struct netdev_rxq *); > int netdev_rxq_recv(struct netdev_rxq *rx, struct dp_packet_batch *); > void netdev_rxq_wait(struct netdev_rxq *); > int netdev_rxq_drain(struct netdev_rxq *); > +int netdev_rxq_length(struct netdev_rxq *rx); argument's name not needed here. > > /* Packet transmission. */ > int netdev_send(struct netdev *, int qid, struct dp_packet_batch *, > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
