| Message ID | 20190702003230.22351-3-ktraynor@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | vhost tx retry updates | expand |
On 7/2/2019 1:32 AM, Kevin Traynor wrote: > vhost tx retries may occur, and it can be a sign that > the guest is not optimally configured. > > Add a custom stat so a user will know if vhost tx retries are > occurring and hence give a hint that guest config should be > examined. > Thanks Kevin, tests ok for me. Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). One other minor comment below. > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > Documentation/topics/dpdk/vhost-user.rst | 5 ++++ > lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst > index 5f393aced..368f44520 100644 > --- a/Documentation/topics/dpdk/vhost-user.rst > +++ b/Documentation/topics/dpdk/vhost-user.rst > @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and > processing packets at the required rate. > > +The amount of Tx retries on a vhost-user or vhost-user-client interface can be > +shown with:: > + > + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries > + > vhost-user Dequeue Zero Copy (experimental) > ------------------------------------------- > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index a380b6ffe..d3e02d389 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) > #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" > > +/* Size of vHost custom stats. */ > +#define VHOST_STAT_TX_RETRIES_SIZE 1 > + > +/* Name of vHost custom stats. */ > +#define VHOST_STAT_TX_RETRIES "tx_retries" > + The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. Regards Ian > #define SOCKET0 0 > > @@ -434,7 +440,9 @@ struct netdev_dpdk { > PADDED_MEMBERS(CACHE_LINE_SIZE, > struct netdev_stats stats; > + /* Custom stat for retries when unable to transmit. */ > + uint64_t tx_retries; > /* Protects stats */ > rte_spinlock_t stats_lock; > - /* 44 pad bytes here. */ > + /* 4 pad bytes here. */ > ); > > @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, > dev->rte_xstats_ids_size = 0; > > + dev->tx_retries = 0; > + > return 0; > } > @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, > netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, > cnt + dropped); > + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); > rte_spinlock_unlock(&dev->stats_lock); > > @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, > } > > +static int > +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, > + struct netdev_custom_stats *custom_stats) > +{ > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + > + ovs_mutex_lock(&dev->mutex); > + > + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; > + custom_stats->counters = (struct netdev_custom_counter *) > + xzalloc(sizeof(struct netdev_custom_counter)); > + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, > + NETDEV_CUSTOM_STATS_NAME_SIZE); > + > + rte_spinlock_lock(&dev->stats_lock); > + custom_stats->counters->value = dev->tx_retries; > + rte_spinlock_unlock(&dev->stats_lock); > + > + ovs_mutex_unlock(&dev->mutex); > + > + return 0; > +} > + > static int > netdev_dpdk_get_features(const struct netdev *netdev, > @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class = { > .get_carrier = netdev_dpdk_vhost_get_carrier, > .get_stats = netdev_dpdk_vhost_get_stats, > + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > .get_status = netdev_dpdk_vhost_user_get_status, > .reconfigure = netdev_dpdk_vhost_reconfigure, > @@ -4413,4 +4448,5 @@ static const struct netdev_class dpdk_vhost_client_class = { > .get_carrier = netdev_dpdk_vhost_get_carrier, > .get_stats = netdev_dpdk_vhost_get_stats, > + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > .get_status = netdev_dpdk_vhost_user_get_status, > .reconfigure = netdev_dpdk_vhost_client_reconfigure, >
On 03.07.2019 18:03, Ian Stokes wrote: > On 7/2/2019 1:32 AM, Kevin Traynor wrote: >> vhost tx retries may occur, and it can be a sign that >> the guest is not optimally configured. >> >> Add a custom stat so a user will know if vhost tx retries are >> occurring and hence give a hint that guest config should be >> examined. >> > > Thanks Kevin, tests ok for me. > > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. > > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). > > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). I think, It's better to keep this in custom stats section since no other port types are going to use it in a near future. Have a few style/naming comments inline. > > One other minor comment below. > >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> --- >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- >> 2 files changed, 42 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >> index 5f393aced..368f44520 100644 >> --- a/Documentation/topics/dpdk/vhost-user.rst >> +++ b/Documentation/topics/dpdk/vhost-user.rst >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and >> processing packets at the required rate. >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be >> +shown with:: >> + >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries >> + >> vhost-user Dequeue Zero Copy (experimental) >> ------------------------------------------- >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index a380b6ffe..d3e02d389 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" >> +/* Size of vHost custom stats. */ >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. >> + >> +/* Name of vHost custom stats. */ s/Name/Names/ ? >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + > > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. Sure. > > Regards > Ian >> #define SOCKET0 0 >> @@ -434,7 +440,9 @@ struct netdev_dpdk { >> PADDED_MEMBERS(CACHE_LINE_SIZE, >> struct netdev_stats stats; >> + /* Custom stat for retries when unable to transmit. */ >> + uint64_t tx_retries; >> /* Protects stats */ >> rte_spinlock_t stats_lock; >> - /* 44 pad bytes here. */ >> + /* 4 pad bytes here. */ >> ); >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >> dev->rte_xstats_ids_size = 0; >> + dev->tx_retries = 0; >> + >> return 0; >> } >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >> cnt + dropped); >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); >> rte_spinlock_unlock(&dev->stats_lock); >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, >> } >> +static int >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >> + struct netdev_custom_stats *custom_stats) >> +{ >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> + >> + ovs_mutex_lock(&dev->mutex); >> + >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; >> + custom_stats->counters = (struct netdev_custom_counter *) >> + xzalloc(sizeof(struct netdev_custom_counter)); Since we have a _SIZE, we, probably, need to use it while allocation. With this and a few coding-style fixes, this line should look like: custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, sizeof *custom_stats->counters); >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, >> + NETDEV_CUSTOM_STATS_NAME_SIZE); >> + >> + rte_spinlock_lock(&dev->stats_lock); >> + custom_stats->counters->value = dev->tx_retries; >> + rte_spinlock_unlock(&dev->stats_lock); >> + >> + ovs_mutex_unlock(&dev->mutex); >> + >> + return 0; >> +} >> + >> static int >> netdev_dpdk_get_features(const struct netdev *netdev, >> @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class = { >> .get_carrier = netdev_dpdk_vhost_get_carrier, >> .get_stats = netdev_dpdk_vhost_get_stats, >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >> .get_status = netdev_dpdk_vhost_user_get_status, >> .reconfigure = netdev_dpdk_vhost_reconfigure, >> @@ -4413,4 +4448,5 @@ static const struct netdev_class dpdk_vhost_client_class = { >> .get_carrier = netdev_dpdk_vhost_get_carrier, >> .get_stats = netdev_dpdk_vhost_get_stats, >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >> .get_status = netdev_dpdk_vhost_user_get_status, >> .reconfigure = netdev_dpdk_vhost_client_reconfigure, >>
On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com> wrote: > On 03.07.2019 18:03, Ian Stokes wrote: > > On 7/2/2019 1:32 AM, Kevin Traynor wrote: > >> vhost tx retries may occur, and it can be a sign that > >> the guest is not optimally configured. > >> > >> Add a custom stat so a user will know if vhost tx retries are > >> occurring and hence give a hint that guest config should be > >> examined. > >> > > > > Thanks Kevin, tests ok for me. > > > > Just a general comment on the design. In comparison to the previous > approach proposed there seems to be more required with the custom stat > approach below. > > > > This may be ok as the retry is very OVS DPDK specific and doesn;t come > dor lets say DPDK (unlike the XTATS). > > > > @Ilya, whats your thoughts? Is this approach preferable for you rather > than adding it to the general stats for all devices? (in which case I > agree, they will not be used for dpdk types so will not be of use). > > I think, It's better to keep this in custom stats section since > no other port types are going to use it in a near future. > > Have a few style/naming comments inline. > > > > > One other minor comment below. > > > >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > >> --- > >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ > >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- > >> 2 files changed, 42 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/topics/dpdk/vhost-user.rst > b/Documentation/topics/dpdk/vhost-user.rst > >> index 5f393aced..368f44520 100644 > >> --- a/Documentation/topics/dpdk/vhost-user.rst > >> +++ b/Documentation/topics/dpdk/vhost-user.rst > >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores > dedicated for consuming and > >> processing packets at the required rate. > >> +The amount of Tx retries on a vhost-user or vhost-user-client > interface can be > >> +shown with:: > >> + > >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries > >> + > >> vhost-user Dequeue Zero Copy (experimental) > >> ------------------------------------------- > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index a380b6ffe..d3e02d389 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / > ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) > >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" > >> +/* Size of vHost custom stats. */ > >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 > > I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. > +1 > > >> + > >> +/* Name of vHost custom stats. */ > > s/Name/Names/ ? > > >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + > > > > The alignment of the defines above look odd as they are not aligned with > the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on > leave I think this will be ok to change on commit if there are no > objections. > > Sure. > > > > > Regards > > Ian > >> #define SOCKET0 0 > >> @@ -434,7 +440,9 @@ struct netdev_dpdk { > >> PADDED_MEMBERS(CACHE_LINE_SIZE, > >> struct netdev_stats stats; > >> + /* Custom stat for retries when unable to transmit. */ > >> + uint64_t tx_retries; > >> /* Protects stats */ > >> rte_spinlock_t stats_lock; > >> - /* 44 pad bytes here. */ > >> + /* 4 pad bytes here. */ > >> ); > >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, > dpdk_port_t port_no, > >> dev->rte_xstats_ids_size = 0; > >> + dev->tx_retries = 0; > >> + > >> return 0; > >> } > >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, > int qid, > >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, > total_pkts, > >> cnt + dropped); > >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); > >> rte_spinlock_unlock(&dev->stats_lock); > >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct > netdev *netdev, > >> } > >> +static int > >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, > >> + struct netdev_custom_stats > *custom_stats) > >> +{ > >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > >> + > >> + ovs_mutex_lock(&dev->mutex); > >> + > >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; > >> + custom_stats->counters = (struct netdev_custom_counter *) > >> + xzalloc(sizeof(struct > netdev_custom_counter)); > > Since we have a _SIZE, we, probably, need to use it while allocation. > With this and a few coding-style fixes, this line should look like: > > custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, > sizeof *custom_stats->counters); > > Can I suggest? custom_stats->counters = xcalloc(custom_stats->size, sizeof *custom_stats->counters); >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, > >> + NETDEV_CUSTOM_STATS_NAME_SIZE); > >> + > >> + rte_spinlock_lock(&dev->stats_lock); > >> + custom_stats->counters->value = dev->tx_retries; > I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> > >> + rte_spinlock_unlock(&dev->stats_lock); > >> + > >> + ovs_mutex_unlock(&dev->mutex); > >> + > >> + return 0; > >> +} > >> + > >> static int > >> netdev_dpdk_get_features(const struct netdev *netdev, > >> @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class > = { > >> .get_carrier = netdev_dpdk_vhost_get_carrier, > >> .get_stats = netdev_dpdk_vhost_get_stats, > >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > >> .get_status = netdev_dpdk_vhost_user_get_status, > >> .reconfigure = netdev_dpdk_vhost_reconfigure, > >> @@ -4413,4 +4448,5 @@ static const struct netdev_class > dpdk_vhost_client_class = { > >> .get_carrier = netdev_dpdk_vhost_get_carrier, > >> .get_stats = netdev_dpdk_vhost_get_stats, > >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > >> .get_status = netdev_dpdk_vhost_user_get_status, > >> .reconfigure = netdev_dpdk_vhost_client_reconfigure, > >> >
On 04.07.2019 14:06, David Marchand wrote: > > > On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote: > > On 03.07.2019 18:03, Ian Stokes wrote: > > On 7/2/2019 1:32 AM, Kevin Traynor wrote: > >> vhost tx retries may occur, and it can be a sign that > >> the guest is not optimally configured. > >> > >> Add a custom stat so a user will know if vhost tx retries are > >> occurring and hence give a hint that guest config should be > >> examined. > >> > > > > Thanks Kevin, tests ok for me. > > > > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. > > > > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). > > > > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). > > I think, It's better to keep this in custom stats section since > no other port types are going to use it in a near future. > > Have a few style/naming comments inline. > > > > > One other minor comment below. > > > >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto:ktraynor@redhat.com>> > >> --- > >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ > >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- > >> 2 files changed, 42 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst > >> index 5f393aced..368f44520 100644 > >> --- a/Documentation/topics/dpdk/vhost-user.rst > >> +++ b/Documentation/topics/dpdk/vhost-user.rst > >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and > >> processing packets at the required rate. > >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be > >> +shown with:: > >> + > >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries > >> + > >> vhost-user Dequeue Zero Copy (experimental) > >> ------------------------------------------- > >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> index a380b6ffe..d3e02d389 100644 > >> --- a/lib/netdev-dpdk.c > >> +++ b/lib/netdev-dpdk.c > >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) > >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" > >> +/* Size of vHost custom stats. */ > >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 > > I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. > > > +1 > > > > >> + > >> +/* Name of vHost custom stats. */ > > s/Name/Names/ ? > > >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + > > > > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. > > Sure. > > > > > Regards > > Ian > >> #define SOCKET0 0 > >> @@ -434,7 +440,9 @@ struct netdev_dpdk { > >> PADDED_MEMBERS(CACHE_LINE_SIZE, > >> struct netdev_stats stats; > >> + /* Custom stat for retries when unable to transmit. */ > >> + uint64_t tx_retries; > >> /* Protects stats */ > >> rte_spinlock_t stats_lock; > >> - /* 44 pad bytes here. */ > >> + /* 4 pad bytes here. */ > >> ); > >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, > >> dev->rte_xstats_ids_size = 0; > >> + dev->tx_retries = 0; > >> + > >> return 0; > >> } > >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, > >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, > >> cnt + dropped); > >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); > >> rte_spinlock_unlock(&dev->stats_lock); > >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, > >> } > >> +static int > >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, > >> + struct netdev_custom_stats *custom_stats) > >> +{ > >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > >> + > >> + ovs_mutex_lock(&dev->mutex); > >> + > >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; > >> + custom_stats->counters = (struct netdev_custom_counter *) > >> + xzalloc(sizeof(struct netdev_custom_counter)); > > Since we have a _SIZE, we, probably, need to use it while allocation. > With this and a few coding-style fixes, this line should look like: > > custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, > sizeof *custom_stats->counters); > > > Can I suggest? > custom_stats->counters = xcalloc(custom_stats->size, > sizeof *custom_stats->counters); Sure. This looks fine for me too. > > >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, > >> + NETDEV_CUSTOM_STATS_NAME_SIZE); > >> + > >> + rte_spinlock_lock(&dev->stats_lock); > >> + custom_stats->counters->value = dev->tx_retries; > > > > I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> +1. This is also in line with my previous comment about having a _SIZE. > > > > >> + rte_spinlock_unlock(&dev->stats_lock); > >> + > >> + ovs_mutex_unlock(&dev->mutex); > >> + > >> + return 0; > >> +} > >> + > >> static int > >> netdev_dpdk_get_features(const struct netdev *netdev, > >> @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class = { > >> .get_carrier = netdev_dpdk_vhost_get_carrier, > >> .get_stats = netdev_dpdk_vhost_get_stats, > >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > >> .get_status = netdev_dpdk_vhost_user_get_status, > >> .reconfigure = netdev_dpdk_vhost_reconfigure, > >> @@ -4413,4 +4448,5 @@ static const struct netdev_class dpdk_vhost_client_class = { > >> .get_carrier = netdev_dpdk_vhost_get_carrier, > >> .get_stats = netdev_dpdk_vhost_get_stats, > >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, > >> .get_status = netdev_dpdk_vhost_user_get_status, > >> .reconfigure = netdev_dpdk_vhost_client_reconfigure, > >> > > > > > -- > David Marchand
On 7/4/2019 12:18 PM, Ilya Maximets wrote: > On 04.07.2019 14:06, David Marchand wrote: >> >> >> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote: >> >> On 03.07.2019 18:03, Ian Stokes wrote: >> > On 7/2/2019 1:32 AM, Kevin Traynor wrote: >> >> vhost tx retries may occur, and it can be a sign that >> >> the guest is not optimally configured. >> >> >> >> Add a custom stat so a user will know if vhost tx retries are >> >> occurring and hence give a hint that guest config should be >> >> examined. >> >> >> > >> > Thanks Kevin, tests ok for me. >> > >> > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. >> > >> > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). >> > >> > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). >> >> I think, It's better to keep this in custom stats section since >> no other port types are going to use it in a near future. >> >> Have a few style/naming comments inline. The changes below all seem reasonable, I can apply and validate before committing if people are happy with that? Ian >> >> > >> > One other minor comment below. >> > >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto:ktraynor@redhat.com>> >> >> --- >> >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ >> >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- >> >> 2 files changed, 42 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >> >> index 5f393aced..368f44520 100644 >> >> --- a/Documentation/topics/dpdk/vhost-user.rst >> >> +++ b/Documentation/topics/dpdk/vhost-user.rst >> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and >> >> processing packets at the required rate. >> >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be >> >> +shown with:: >> >> + >> >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries >> >> + >> >> vhost-user Dequeue Zero Copy (experimental) >> >> ------------------------------------------- >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> >> index a380b6ffe..d3e02d389 100644 >> >> --- a/lib/netdev-dpdk.c >> >> +++ b/lib/netdev-dpdk.c >> >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) >> >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" >> >> +/* Size of vHost custom stats. */ >> >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 >> >> I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. >> >> >> +1 >> >> >> >> >> + >> >> +/* Name of vHost custom stats. */ >> >> s/Name/Names/ ? >> >> >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + >> > >> > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. >> >> Sure. >> >> > >> > Regards >> > Ian >> >> #define SOCKET0 0 >> >> @@ -434,7 +440,9 @@ struct netdev_dpdk { >> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >> >> struct netdev_stats stats; >> >> + /* Custom stat for retries when unable to transmit. */ >> >> + uint64_t tx_retries; >> >> /* Protects stats */ >> >> rte_spinlock_t stats_lock; >> >> - /* 44 pad bytes here. */ >> >> + /* 4 pad bytes here. */ >> >> ); >> >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >> >> dev->rte_xstats_ids_size = 0; >> >> + dev->tx_retries = 0; >> >> + >> >> return 0; >> >> } >> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >> >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >> >> cnt + dropped); >> >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); >> >> rte_spinlock_unlock(&dev->stats_lock); >> >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, >> >> } >> >> +static int >> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >> >> + struct netdev_custom_stats *custom_stats) >> >> +{ >> >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> >> + >> >> + ovs_mutex_lock(&dev->mutex); >> >> + >> >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; >> >> + custom_stats->counters = (struct netdev_custom_counter *) >> >> + xzalloc(sizeof(struct netdev_custom_counter)); >> >> Since we have a _SIZE, we, probably, need to use it while allocation. >> With this and a few coding-style fixes, this line should look like: >> >> custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, >> sizeof *custom_stats->counters); >> >> >> Can I suggest? >> custom_stats->counters = xcalloc(custom_stats->size, >> sizeof *custom_stats->counters); > > Sure. This looks fine for me too. > >> >> >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, >> >> + NETDEV_CUSTOM_STATS_NAME_SIZE); >> >> + >> >> + rte_spinlock_lock(&dev->stats_lock); >> >> + custom_stats->counters->value = dev->tx_retries; >> >> >> >> I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> > > +1. > This is also in line with my previous comment about having a _SIZE. > >> >> >> >> >> + rte_spinlock_unlock(&dev->stats_lock); >> >> + >> >> + ovs_mutex_unlock(&dev->mutex); >> >> + >> >> + return 0; >> >> +} >> >> + >> >> static int >> >> netdev_dpdk_get_features(const struct netdev *netdev, >> >> @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class = { >> >> .get_carrier = netdev_dpdk_vhost_get_carrier, >> >> .get_stats = netdev_dpdk_vhost_get_stats, >> >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >> >> .get_status = netdev_dpdk_vhost_user_get_status, >> >> .reconfigure = netdev_dpdk_vhost_reconfigure, >> >> @@ -4413,4 +4448,5 @@ static const struct netdev_class dpdk_vhost_client_class = { >> >> .get_carrier = netdev_dpdk_vhost_get_carrier, >> >> .get_stats = netdev_dpdk_vhost_get_stats, >> >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >> >> .get_status = netdev_dpdk_vhost_user_get_status, >> >> .reconfigure = netdev_dpdk_vhost_client_reconfigure, >> >> >> >> >> >> >> -- >> David Marchand
On 04.07.2019 17:04, Ian Stokes wrote: > On 7/4/2019 12:18 PM, Ilya Maximets wrote: >> On 04.07.2019 14:06, David Marchand wrote: >>> >>> >>> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote: >>> >>> On 03.07.2019 18:03, Ian Stokes wrote: >>> > On 7/2/2019 1:32 AM, Kevin Traynor wrote: >>> >> vhost tx retries may occur, and it can be a sign that >>> >> the guest is not optimally configured. >>> >> >>> >> Add a custom stat so a user will know if vhost tx retries are >>> >> occurring and hence give a hint that guest config should be >>> >> examined. >>> >> >>> > >>> > Thanks Kevin, tests ok for me. >>> > >>> > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. >>> > >>> > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). >>> > >>> > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). >>> >>> I think, It's better to keep this in custom stats section since >>> no other port types are going to use it in a near future. >>> >>> Have a few style/naming comments inline. > > The changes below all seem reasonable, I can apply and validate before committing if people are happy with that? OK. > > Ian > >>> >>> > >>> > One other minor comment below. >>> > >>> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto:ktraynor@redhat.com>> >>> >> --- >>> >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ >>> >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- >>> >> 2 files changed, 42 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>> >> index 5f393aced..368f44520 100644 >>> >> --- a/Documentation/topics/dpdk/vhost-user.rst >>> >> +++ b/Documentation/topics/dpdk/vhost-user.rst >>> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and >>> >> processing packets at the required rate. >>> >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be >>> >> +shown with:: >>> >> + >>> >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries >>> >> + >>> >> vhost-user Dequeue Zero Copy (experimental) >>> >> ------------------------------------------- >>> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> >> index a380b6ffe..d3e02d389 100644 >>> >> --- a/lib/netdev-dpdk.c >>> >> +++ b/lib/netdev-dpdk.c >>> >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) >>> >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" >>> >> +/* Size of vHost custom stats. */ >>> >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 >>> >>> I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. >>> >>> >>> +1 >>> >>> >>> >>> >> + >>> >> +/* Name of vHost custom stats. */ >>> >>> s/Name/Names/ ? >>> >>> >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + >>> > >>> > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. >>> >>> Sure. >>> >>> > >>> > Regards >>> > Ian >>> >> #define SOCKET0 0 >>> >> @@ -434,7 +440,9 @@ struct netdev_dpdk { >>> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >>> >> struct netdev_stats stats; >>> >> + /* Custom stat for retries when unable to transmit. */ >>> >> + uint64_t tx_retries; >>> >> /* Protects stats */ >>> >> rte_spinlock_t stats_lock; >>> >> - /* 44 pad bytes here. */ >>> >> + /* 4 pad bytes here. */ >>> >> ); >>> >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >>> >> dev->rte_xstats_ids_size = 0; >>> >> + dev->tx_retries = 0; >>> >> + >>> >> return 0; >>> >> } >>> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >>> >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >>> >> cnt + dropped); >>> >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); >>> >> rte_spinlock_unlock(&dev->stats_lock); >>> >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, >>> >> } >>> >> +static int >>> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >>> >> + struct netdev_custom_stats *custom_stats) >>> >> +{ >>> >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >>> >> + >>> >> + ovs_mutex_lock(&dev->mutex); >>> >> + >>> >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; >>> >> + custom_stats->counters = (struct netdev_custom_counter *) >>> >> + xzalloc(sizeof(struct netdev_custom_counter)); >>> >>> Since we have a _SIZE, we, probably, need to use it while allocation. >>> With this and a few coding-style fixes, this line should look like: >>> >>> custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, >>> sizeof *custom_stats->counters); >>> >>> >>> Can I suggest? >>> custom_stats->counters = xcalloc(custom_stats->size, >>> sizeof *custom_stats->counters); >> >> Sure. This looks fine for me too. >> >>> >>> >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, >>> >> + NETDEV_CUSTOM_STATS_NAME_SIZE); >>> >> + >>> >> + rte_spinlock_lock(&dev->stats_lock); >>> >> + custom_stats->counters->value = dev->tx_retries; >>> >>> >>> >>> I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> >> >> +1. >> This is also in line with my previous comment about having a _SIZE. >> >>> >>> >>> >> + rte_spinlock_unlock(&dev->stats_lock); >>> >> + >>> >> + ovs_mutex_unlock(&dev->mutex); >>> >> + >>> >> + return 0; >>> >> +} >>> >> + >>> >> static int >>> >> netdev_dpdk_get_features(const struct netdev *netdev, >>> >> @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class = { >>> >> .get_carrier = netdev_dpdk_vhost_get_carrier, >>> >> .get_stats = netdev_dpdk_vhost_get_stats, >>> >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >>> >> .get_status = netdev_dpdk_vhost_user_get_status, >>> >> .reconfigure = netdev_dpdk_vhost_reconfigure, >>> >> @@ -4413,4 +4448,5 @@ static const struct netdev_class dpdk_vhost_client_class = { >>> >> .get_carrier = netdev_dpdk_vhost_get_carrier, >>> >> .get_stats = netdev_dpdk_vhost_get_stats, >>> >> + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, >>> >> .get_status = netdev_dpdk_vhost_user_get_status, >>> >> .reconfigure = netdev_dpdk_vhost_client_reconfigure, >>> >> >>> >>> >>> >>> >>> -- >>> David Marchand > > >
On 7/4/2019 12:18 PM, Ilya Maximets wrote: > On 04.07.2019 14:06, David Marchand wrote: >> >> >> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote: >> >> On 03.07.2019 18:03, Ian Stokes wrote: >> > On 7/2/2019 1:32 AM, Kevin Traynor wrote: >> >> vhost tx retries may occur, and it can be a sign that >> >> the guest is not optimally configured. >> >> >> >> Add a custom stat so a user will know if vhost tx retries are >> >> occurring and hence give a hint that guest config should be >> >> examined. >> >> >> > >> > Thanks Kevin, tests ok for me. >> > >> > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. >> > >> > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). >> > >> > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). >> >> I think, It's better to keep this in custom stats section since >> no other port types are going to use it in a near future. >> >> Have a few style/naming comments inline. >> >> > >> > One other minor comment below. >> > >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto:ktraynor@redhat.com>> >> >> --- >> >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ >> >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- >> >> 2 files changed, 42 insertions(+), 1 deletion(-) >> >> >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >> >> index 5f393aced..368f44520 100644 >> >> --- a/Documentation/topics/dpdk/vhost-user.rst >> >> +++ b/Documentation/topics/dpdk/vhost-user.rst >> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and >> >> processing packets at the required rate. >> >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be >> >> +shown with:: >> >> + >> >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries >> >> + >> >> vhost-user Dequeue Zero Copy (experimental) >> >> ------------------------------------------- >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> >> index a380b6ffe..d3e02d389 100644 >> >> --- a/lib/netdev-dpdk.c >> >> +++ b/lib/netdev-dpdk.c >> >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) >> >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" >> >> +/* Size of vHost custom stats. */ >> >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 >> >> I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. >> >> >> +1 >> >> >> >> >> + >> >> +/* Name of vHost custom stats. */ >> >> s/Name/Names/ ? >> >> >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + >> > >> > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. >> >> Sure. >> >> > >> > Regards >> > Ian >> >> #define SOCKET0 0 >> >> @@ -434,7 +440,9 @@ struct netdev_dpdk { >> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >> >> struct netdev_stats stats; >> >> + /* Custom stat for retries when unable to transmit. */ >> >> + uint64_t tx_retries; >> >> /* Protects stats */ >> >> rte_spinlock_t stats_lock; >> >> - /* 44 pad bytes here. */ >> >> + /* 4 pad bytes here. */ >> >> ); >> >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >> >> dev->rte_xstats_ids_size = 0; >> >> + dev->tx_retries = 0; >> >> + >> >> return 0; >> >> } >> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >> >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >> >> cnt + dropped); >> >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); >> >> rte_spinlock_unlock(&dev->stats_lock); >> >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, >> >> } >> >> +static int >> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >> >> + struct netdev_custom_stats *custom_stats) >> >> +{ >> >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> >> + >> >> + ovs_mutex_lock(&dev->mutex); >> >> + >> >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; >> >> + custom_stats->counters = (struct netdev_custom_counter *) >> >> + xzalloc(sizeof(struct netdev_custom_counter)); >> >> Since we have a _SIZE, we, probably, need to use it while allocation. >> With this and a few coding-style fixes, this line should look like: >> >> custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, >> sizeof *custom_stats->counters); >> >> >> Can I suggest? >> custom_stats->counters = xcalloc(custom_stats->size, >> sizeof *custom_stats->counters); > > Sure. This looks fine for me too. > >> >> >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, >> >> + NETDEV_CUSTOM_STATS_NAME_SIZE); >> >> + >> >> + rte_spinlock_lock(&dev->stats_lock); >> >> + custom_stats->counters->value = dev->tx_retries; >> >> >> >> I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> > > +1. > This is also in line with my previous comment about having a _SIZE. Just to clarify above, maybe I misunderstood what you meant, but are you suggesting custom_stats->counters[0] = dev->tx_retries; I don't think that will work as it will be an incorrect type assignment right? Regards Ian
On Thu, Jul 4, 2019 at 5:13 PM Ian Stokes <ian.stokes@intel.com> wrote: > On 7/4/2019 12:18 PM, Ilya Maximets wrote: > > On 04.07.2019 14:06, David Marchand wrote: > >> > >> > >> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com > <mailto:i.maximets@samsung.com>> wrote: > >> > >> On 03.07.2019 18:03, Ian Stokes wrote: > >> > On 7/2/2019 1:32 AM, Kevin Traynor wrote: > >> >> vhost tx retries may occur, and it can be a sign that > >> >> the guest is not optimally configured. > >> >> > >> >> Add a custom stat so a user will know if vhost tx retries are > >> >> occurring and hence give a hint that guest config should be > >> >> examined. > >> >> > >> > > >> > Thanks Kevin, tests ok for me. > >> > > >> > Just a general comment on the design. In comparison to the > previous approach proposed there seems to be more required with the custom > stat approach below. > >> > > >> > This may be ok as the retry is very OVS DPDK specific and > doesn;t come dor lets say DPDK (unlike the XTATS). > >> > > >> > @Ilya, whats your thoughts? Is this approach preferable for you > rather than adding it to the general stats for all devices? (in which case > I agree, they will not be used for dpdk types so will not be of use). > >> > >> I think, It's better to keep this in custom stats section since > >> no other port types are going to use it in a near future. > >> > >> Have a few style/naming comments inline. > >> > >> > > >> > One other minor comment below. > >> > > >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto: > ktraynor@redhat.com>> > >> >> --- > >> >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ > >> >> lib/netdev-dpdk.c | 38 > +++++++++++++++++++++++- > >> >> 2 files changed, 42 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst > b/Documentation/topics/dpdk/vhost-user.rst > >> >> index 5f393aced..368f44520 100644 > >> >> --- a/Documentation/topics/dpdk/vhost-user.rst > >> >> +++ b/Documentation/topics/dpdk/vhost-user.rst > >> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores > dedicated for consuming and > >> >> processing packets at the required rate. > >> >> +The amount of Tx retries on a vhost-user or > vhost-user-client interface can be > >> >> +shown with:: > >> >> + > >> >> + $ ovs-vsctl get Interface dpdkvhostclient0 > statistics:tx_retries > >> >> + > >> >> vhost-user Dequeue Zero Copy (experimental) > >> >> ------------------------------------------- > >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > >> >> index a380b6ffe..d3e02d389 100644 > >> >> --- a/lib/netdev-dpdk.c > >> >> +++ b/lib/netdev-dpdk.c > >> >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / > ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) > >> >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" > >> >> +/* Size of vHost custom stats. */ > >> >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 > >> > >> I think, it's better to rename to something like > VHOST_CUSTOM_STATS_SIZE. > >> > >> > >> +1 > >> > >> > >> > >> >> + > >> >> +/* Name of vHost custom stats. */ > >> > >> s/Name/Names/ ? > >> > >> >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + > >> > > >> > The alignment of the defines above look odd as they are not > aligned with the existing DPDK XSTAT name definitions. It's only minor, as > Kevin is on leave I think this will be ok to change on commit if there are > no objections. > >> > >> Sure. > >> > >> > > >> > Regards > >> > Ian > >> >> #define SOCKET0 0 > >> >> @@ -434,7 +440,9 @@ struct netdev_dpdk { > >> >> PADDED_MEMBERS(CACHE_LINE_SIZE, > >> >> struct netdev_stats stats; > >> >> + /* Custom stat for retries when unable to transmit. */ > >> >> + uint64_t tx_retries; > >> >> /* Protects stats */ > >> >> rte_spinlock_t stats_lock; > >> >> - /* 44 pad bytes here. */ > >> >> + /* 4 pad bytes here. */ > >> >> ); > >> >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, > dpdk_port_t port_no, > >> >> dev->rte_xstats_ids_size = 0; > >> >> + dev->tx_retries = 0; > >> >> + > >> >> return 0; > >> >> } > >> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev > *netdev, int qid, > >> >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, > total_pkts, > >> >> cnt + dropped); > >> >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); > >> >> rte_spinlock_unlock(&dev->stats_lock); > >> >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const > struct netdev *netdev, > >> >> } > >> >> +static int > >> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, > >> >> + struct netdev_custom_stats > *custom_stats) > >> >> +{ > >> >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > >> >> + > >> >> + ovs_mutex_lock(&dev->mutex); > >> >> + > >> >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; > >> >> + custom_stats->counters = (struct netdev_custom_counter *) > >> >> + xzalloc(sizeof(struct > netdev_custom_counter)); > >> > >> Since we have a _SIZE, we, probably, need to use it while > allocation. > >> With this and a few coding-style fixes, this line should look like: > >> > >> custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, > >> sizeof > *custom_stats->counters); > >> > >> > >> Can I suggest? > >> custom_stats->counters = xcalloc(custom_stats->size, > >> sizeof *custom_stats->counters); > > > > Sure. This looks fine for me too. > > > >> > >> >> + ovs_strlcpy(custom_stats->counters->name, > VHOST_STAT_TX_RETRIES, > >> >> + NETDEV_CUSTOM_STATS_NAME_SIZE); > >> >> + > >> >> + rte_spinlock_lock(&dev->stats_lock); > >> >> + custom_stats->counters->value = dev->tx_retries; > >> > >> > >> > >> I have a patch after this that will add another stats, so I'd rather > see custom_stats->counters[0]. than custom_stats->counters-> > > > > +1. > > This is also in line with my previous comment about having a _SIZE. > > Just to clarify above, maybe I misunderstood what you meant, but are you > suggesting > > custom_stats->counters[0] = dev->tx_retries; > > I don't think that will work as it will be an incorrect type assignment > right? > > I meant: ovs_strlcpy(custom_stats->counters[0].name, VHOST_STAT_TX_RETRIES, NETDEV_CUSTOM_STATS_NAME_SIZE); custom_stats->counters[0].value = dev->tx_retries;
On 04.07.2019 18:13, Ian Stokes wrote: > On 7/4/2019 12:18 PM, Ilya Maximets wrote: >> On 04.07.2019 14:06, David Marchand wrote: >>> >>> >>> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote: >>> >>> On 03.07.2019 18:03, Ian Stokes wrote: >>> > On 7/2/2019 1:32 AM, Kevin Traynor wrote: >>> >> vhost tx retries may occur, and it can be a sign that >>> >> the guest is not optimally configured. >>> >> >>> >> Add a custom stat so a user will know if vhost tx retries are >>> >> occurring and hence give a hint that guest config should be >>> >> examined. >>> >> >>> > >>> > Thanks Kevin, tests ok for me. >>> > >>> > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. >>> > >>> > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). >>> > >>> > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). >>> >>> I think, It's better to keep this in custom stats section since >>> no other port types are going to use it in a near future. >>> >>> Have a few style/naming comments inline. >>> >>> > >>> > One other minor comment below. >>> > >>> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto:ktraynor@redhat.com>> >>> >> --- >>> >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ >>> >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- >>> >> 2 files changed, 42 insertions(+), 1 deletion(-) >>> >> >>> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>> >> index 5f393aced..368f44520 100644 >>> >> --- a/Documentation/topics/dpdk/vhost-user.rst >>> >> +++ b/Documentation/topics/dpdk/vhost-user.rst >>> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and >>> >> processing packets at the required rate. >>> >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be >>> >> +shown with:: >>> >> + >>> >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries >>> >> + >>> >> vhost-user Dequeue Zero Copy (experimental) >>> >> ------------------------------------------- >>> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> >> index a380b6ffe..d3e02d389 100644 >>> >> --- a/lib/netdev-dpdk.c >>> >> +++ b/lib/netdev-dpdk.c >>> >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) >>> >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" >>> >> +/* Size of vHost custom stats. */ >>> >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 >>> >>> I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. >>> >>> >>> +1 >>> >>> >>> >>> >> + >>> >> +/* Name of vHost custom stats. */ >>> >>> s/Name/Names/ ? >>> >>> >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + >>> > >>> > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. >>> >>> Sure. >>> >>> > >>> > Regards >>> > Ian >>> >> #define SOCKET0 0 >>> >> @@ -434,7 +440,9 @@ struct netdev_dpdk { >>> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >>> >> struct netdev_stats stats; >>> >> + /* Custom stat for retries when unable to transmit. */ >>> >> + uint64_t tx_retries; >>> >> /* Protects stats */ >>> >> rte_spinlock_t stats_lock; >>> >> - /* 44 pad bytes here. */ >>> >> + /* 4 pad bytes here. */ >>> >> ); >>> >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >>> >> dev->rte_xstats_ids_size = 0; >>> >> + dev->tx_retries = 0; >>> >> + >>> >> return 0; >>> >> } >>> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >>> >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >>> >> cnt + dropped); >>> >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); >>> >> rte_spinlock_unlock(&dev->stats_lock); >>> >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, >>> >> } >>> >> +static int >>> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >>> >> + struct netdev_custom_stats *custom_stats) >>> >> +{ >>> >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >>> >> + >>> >> + ovs_mutex_lock(&dev->mutex); >>> >> + >>> >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; >>> >> + custom_stats->counters = (struct netdev_custom_counter *) >>> >> + xzalloc(sizeof(struct netdev_custom_counter)); >>> >>> Since we have a _SIZE, we, probably, need to use it while allocation. >>> With this and a few coding-style fixes, this line should look like: >>> >>> custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, >>> sizeof *custom_stats->counters); >>> >>> >>> Can I suggest? >>> custom_stats->counters = xcalloc(custom_stats->size, >>> sizeof *custom_stats->counters); >> >> Sure. This looks fine for me too. >> >>> >>> >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, >>> >> + NETDEV_CUSTOM_STATS_NAME_SIZE); >>> >> + >>> >> + rte_spinlock_lock(&dev->stats_lock); >>> >> + custom_stats->counters->value = dev->tx_retries; >>> >>> >>> >>> I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> >> >> +1. >> This is also in line with my previous comment about having a _SIZE. > > Just to clarify above, maybe I misunderstood what you meant, but are you suggesting > > custom_stats->counters[0] = dev->tx_retries; > > I don't think that will work as it will be an incorrect type assignment right? custom_stats->counters[0].value = dev->tx_retries;
On 7/4/2019 4:15 PM, Ilya Maximets wrote: > On 04.07.2019 18:13, Ian Stokes wrote: >> On 7/4/2019 12:18 PM, Ilya Maximets wrote: >>> On 04.07.2019 14:06, David Marchand wrote: >>>> >>>> >>>> On Thu, Jul 4, 2019 at 12:42 PM Ilya Maximets <i.maximets@samsung.com <mailto:i.maximets@samsung.com>> wrote: >>>> >>>> On 03.07.2019 18:03, Ian Stokes wrote: >>>> > On 7/2/2019 1:32 AM, Kevin Traynor wrote: >>>> >> vhost tx retries may occur, and it can be a sign that >>>> >> the guest is not optimally configured. >>>> >> >>>> >> Add a custom stat so a user will know if vhost tx retries are >>>> >> occurring and hence give a hint that guest config should be >>>> >> examined. >>>> >> >>>> > >>>> > Thanks Kevin, tests ok for me. >>>> > >>>> > Just a general comment on the design. In comparison to the previous approach proposed there seems to be more required with the custom stat approach below. >>>> > >>>> > This may be ok as the retry is very OVS DPDK specific and doesn;t come dor lets say DPDK (unlike the XTATS). >>>> > >>>> > @Ilya, whats your thoughts? Is this approach preferable for you rather than adding it to the general stats for all devices? (in which case I agree, they will not be used for dpdk types so will not be of use). >>>> >>>> I think, It's better to keep this in custom stats section since >>>> no other port types are going to use it in a near future. >>>> >>>> Have a few style/naming comments inline. >>>> >>>> > >>>> > One other minor comment below. >>>> > >>>> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com <mailto:ktraynor@redhat.com>> >>>> >> --- >>>> >> Documentation/topics/dpdk/vhost-user.rst | 5 ++++ >>>> >> lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- >>>> >> 2 files changed, 42 insertions(+), 1 deletion(-) >>>> >> >>>> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>> >> index 5f393aced..368f44520 100644 >>>> >> --- a/Documentation/topics/dpdk/vhost-user.rst >>>> >> +++ b/Documentation/topics/dpdk/vhost-user.rst >>>> >> @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and >>>> >> processing packets at the required rate. >>>> >> +The amount of Tx retries on a vhost-user or vhost-user-client interface can be >>>> >> +shown with:: >>>> >> + >>>> >> + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries >>>> >> + >>>> >> vhost-user Dequeue Zero Copy (experimental) >>>> >> ------------------------------------------- >>>> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>>> >> index a380b6ffe..d3e02d389 100644 >>>> >> --- a/lib/netdev-dpdk.c >>>> >> +++ b/lib/netdev-dpdk.c >>>> >> @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) >>>> >> #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" >>>> >> +/* Size of vHost custom stats. */ >>>> >> +#define VHOST_STAT_TX_RETRIES_SIZE 1 >>>> >>>> I think, it's better to rename to something like VHOST_CUSTOM_STATS_SIZE. >>>> >>>> >>>> +1 >>>> >>>> >>>> >>>> >> + >>>> >> +/* Name of vHost custom stats. */ >>>> >>>> s/Name/Names/ ? >>>> >>>> >> +#define VHOST_STAT_TX_RETRIES "tx_retries" > + >>>> > >>>> > The alignment of the defines above look odd as they are not aligned with the existing DPDK XSTAT name definitions. It's only minor, as Kevin is on leave I think this will be ok to change on commit if there are no objections. >>>> >>>> Sure. >>>> >>>> > >>>> > Regards >>>> > Ian >>>> >> #define SOCKET0 0 >>>> >> @@ -434,7 +440,9 @@ struct netdev_dpdk { >>>> >> PADDED_MEMBERS(CACHE_LINE_SIZE, >>>> >> struct netdev_stats stats; >>>> >> + /* Custom stat for retries when unable to transmit. */ >>>> >> + uint64_t tx_retries; >>>> >> /* Protects stats */ >>>> >> rte_spinlock_t stats_lock; >>>> >> - /* 44 pad bytes here. */ >>>> >> + /* 4 pad bytes here. */ >>>> >> ); >>>> >> @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, >>>> >> dev->rte_xstats_ids_size = 0; >>>> >> + dev->tx_retries = 0; >>>> >> + >>>> >> return 0; >>>> >> } >>>> >> @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >>>> >> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, >>>> >> cnt + dropped); >>>> >> + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); >>>> >> rte_spinlock_unlock(&dev->stats_lock); >>>> >> @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, >>>> >> } >>>> >> +static int >>>> >> +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, >>>> >> + struct netdev_custom_stats *custom_stats) >>>> >> +{ >>>> >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >>>> >> + >>>> >> + ovs_mutex_lock(&dev->mutex); >>>> >> + >>>> >> + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; >>>> >> + custom_stats->counters = (struct netdev_custom_counter *) >>>> >> + xzalloc(sizeof(struct netdev_custom_counter)); >>>> >>>> Since we have a _SIZE, we, probably, need to use it while allocation. >>>> With this and a few coding-style fixes, this line should look like: >>>> >>>> custom_stats->counters = xcalloc(VHOST_STAT_TX_RETRIES_SIZE, >>>> sizeof *custom_stats->counters); >>>> >>>> >>>> Can I suggest? >>>> custom_stats->counters = xcalloc(custom_stats->size, >>>> sizeof *custom_stats->counters); >>> >>> Sure. This looks fine for me too. >>> >>>> >>>> >> + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, >>>> >> + NETDEV_CUSTOM_STATS_NAME_SIZE); >>>> >> + >>>> >> + rte_spinlock_lock(&dev->stats_lock); >>>> >> + custom_stats->counters->value = dev->tx_retries; >>>> >>>> >>>> >>>> I have a patch after this that will add another stats, so I'd rather see custom_stats->counters[0]. than custom_stats->counters-> >>> >>> +1. >>> This is also in line with my previous comment about having a _SIZE. >> >> Just to clarify above, maybe I misunderstood what you meant, but are you suggesting >> >> custom_stats->counters[0] = dev->tx_retries; >> >> I don't think that will work as it will be an incorrect type assignment right? > > custom_stats->counters[0].value = dev->tx_retries; > :D, thats fine, what I thought but wanted to confirm. Ian
diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst index 5f393aced..368f44520 100644 --- a/Documentation/topics/dpdk/vhost-user.rst +++ b/Documentation/topics/dpdk/vhost-user.rst @@ -521,4 +521,9 @@ The guest should also have sufficient cores dedicated for consuming and processing packets at the required rate. +The amount of Tx retries on a vhost-user or vhost-user-client interface can be +shown with:: + + $ ovs-vsctl get Interface dpdkvhostclient0 statistics:tx_retries + vhost-user Dequeue Zero Copy (experimental) ------------------------------------------- diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index a380b6ffe..d3e02d389 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -139,4 +139,10 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF / MIN_NB_MBUF)) #define XSTAT_RX_JABBER_ERRORS "rx_jabber_errors" +/* Size of vHost custom stats. */ +#define VHOST_STAT_TX_RETRIES_SIZE 1 + +/* Name of vHost custom stats. */ +#define VHOST_STAT_TX_RETRIES "tx_retries" + #define SOCKET0 0 @@ -434,7 +440,9 @@ struct netdev_dpdk { PADDED_MEMBERS(CACHE_LINE_SIZE, struct netdev_stats stats; + /* Custom stat for retries when unable to transmit. */ + uint64_t tx_retries; /* Protects stats */ rte_spinlock_t stats_lock; - /* 44 pad bytes here. */ + /* 4 pad bytes here. */ ); @@ -1190,4 +1198,6 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no, dev->rte_xstats_ids_size = 0; + dev->tx_retries = 0; + return 0; } @@ -2381,4 +2391,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts, cnt + dropped); + dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM); rte_spinlock_unlock(&dev->stats_lock); @@ -2818,4 +2829,27 @@ netdev_dpdk_get_custom_stats(const struct netdev *netdev, } +static int +netdev_dpdk_vhost_get_custom_stats(const struct netdev *netdev, + struct netdev_custom_stats *custom_stats) +{ + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); + + ovs_mutex_lock(&dev->mutex); + + custom_stats->size = VHOST_STAT_TX_RETRIES_SIZE; + custom_stats->counters = (struct netdev_custom_counter *) + xzalloc(sizeof(struct netdev_custom_counter)); + ovs_strlcpy(custom_stats->counters->name, VHOST_STAT_TX_RETRIES, + NETDEV_CUSTOM_STATS_NAME_SIZE); + + rte_spinlock_lock(&dev->stats_lock); + custom_stats->counters->value = dev->tx_retries; + rte_spinlock_unlock(&dev->stats_lock); + + ovs_mutex_unlock(&dev->mutex); + + return 0; +} + static int netdev_dpdk_get_features(const struct netdev *netdev, @@ -4398,4 +4432,5 @@ static const struct netdev_class dpdk_vhost_class = { .get_carrier = netdev_dpdk_vhost_get_carrier, .get_stats = netdev_dpdk_vhost_get_stats, + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, .get_status = netdev_dpdk_vhost_user_get_status, .reconfigure = netdev_dpdk_vhost_reconfigure, @@ -4413,4 +4448,5 @@ static const struct netdev_class dpdk_vhost_client_class = { .get_carrier = netdev_dpdk_vhost_get_carrier, .get_stats = netdev_dpdk_vhost_get_stats, + .get_custom_stats = netdev_dpdk_vhost_get_custom_stats, .get_status = netdev_dpdk_vhost_user_get_status, .reconfigure = netdev_dpdk_vhost_client_reconfigure,
vhost tx retries may occur, and it can be a sign that the guest is not optimally configured. Add a custom stat so a user will know if vhost tx retries are occurring and hence give a hint that guest config should be examined. Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- Documentation/topics/dpdk/vhost-user.rst | 5 ++++ lib/netdev-dpdk.c | 38 +++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-)