Thanks for the review. Answers inline. Regards, Jan
> From: Ilya Maximets [mailto:[email protected]] > Sent: Wednesday, 17 January, 2018 11:47 > Subject: Re: [PATCH v7 1/3] netdev: Add rxq callback function rxq_length() > > 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. OK. > > > > > 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. OK. Not necessary for our use case, as it will only be called by the PMD after having received a full batch of 32 packets, but in general I agree those checks are needed. > > > + /* 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. I'm curious: is there any difference regarding thread-safeness and use during device configuration compared to the other netdev rxq functions? Why would it be necessary to list these constraints for this function but not the others? And why should this read-only function be a priori thread-unsafe? And in which sense: concurrent invocations of this function? Or concurrent invocation of this function with rxq_recv() and rxq_drain()? Should the netdev.h and the netdev_provider.h not rather have a general disclaimer that the rxq functions are only to be called from a single thread assigned "polling" the rx queue, or alternatively be protected by a lock on the netdev user side? > > > + 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. */ OK. > > > +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. OK. > > > > > /* 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
