| Message ID | 20190621134158.7000-1-ktraynor@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/3] netdev-dpdk: Fix and document vhost tx retries. | expand |
Bleep bloop. Greetings Kevin Traynor, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Line is 81 characters long (recommended limit is 79)
#44 FILE: Documentation/topics/dpdk/vhost-user.rst:98:
<address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/>
Lines checked: 73, Warnings: 1, Errors: 0
Please check this out. If you feel there has been an error, please email aconole@bytheb.org
Thanks,
0-day Robot
On Fri, Jun 21, 2019 at 02:41:56PM +0100, Kevin Traynor wrote: > Fix minor issue of one additional retry and add > documentation about vhost tx retries and ways to > reduce/remove them. > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > > --- > There is a checkpatch warning that one of the libvirt > lines in docs is a few characters too long. It is similar > for some others in the file and I think it makes sense to > leave it as is but can wrap if required. > --- > Documentation/topics/dpdk/vhost-user.rst | 31 ++++++++++++++++++++++++ > lib/netdev-dpdk.c | 2 +- > 2 files changed, 32 insertions(+), 1 deletion(-) > > diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst > index f7b4b338e..d1682fd05 100644 > --- a/Documentation/topics/dpdk/vhost-user.rst > +++ b/Documentation/topics/dpdk/vhost-user.rst > @@ -76,4 +76,35 @@ mode ports require QEMU version 2.7. Ports of type vhost-user are currently > deprecated and will be removed in a future release. > > +vhost tx retries > +~~~~~~~~~~~~~~~~ > + > +When sending a batch of packets (max is 32) to a vhost-user or Maybe use the actual define name (NETDEV_MAX_BURST) since it might change? > +vhost-user-client interface, it may happen that some but not all of the packets > +in the batch are able to be sent to the guest. This is often because there is > +not enough free descriptors in the virtqueue for all the packets to be sent. > +In this case there will be a retry, with a default maximum of 8 occurring. If > +at any time no packets can be sent, it may mean the guest is not accepting > +packets, so there are no (more) retries. > + > +Tx Retries may be reduced or removed by some external configuration, such s/removed/even avoided/ ? > +as increasing the virtqueue size through the ``rx_queue_size`` parameter in > +libvirt:: > + > + <interface type='vhostuser'> > + <mac address='56:48:4f:53:54:01'/> > + <source type='unix' path='/tmp/dpdkvhostclient0' mode='server'/> > + <model type='virtio'/> > + <driver name='vhost' rx_queue_size='1024' tx_queue_size='1024'/> > + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> > + </interface> Do we need to worry about the qemu version? > + > +The guest application will also need need to provide enough descriptors. For > +example with ``testpmd`` the command line argument can be used:: > + > + --rxd=1024 --txd=1024 > + > +The guest should also have sufficient cores dedicated for consuming and > +processing packets at the required rate. > + > .. _dpdk-vhost-user: > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 4856c56aa..0f3b9c6f4 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -2353,5 +2353,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, > break; > } > - } while (cnt && (retries++ <= VHOST_ENQ_RETRY_NUM)); > + } while (cnt && (retries++ < VHOST_ENQ_RETRY_NUM)); You're removing the packet's last hope to reach home, are you sure? :-) fbl > > rte_spinlock_unlock(&dev->tx_q[qid].tx_lock); > -- > 2.20.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 21 Jun 2019, at 15:41, Kevin Traynor wrote: > Fix minor issue of one additional retry and add > documentation about vhost tx retries and ways to > reduce/remove them. > > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > <SNIP> Change looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 24/06/2019 22:40, Flavio Leitner wrote: > On Fri, Jun 21, 2019 at 02:41:56PM +0100, Kevin Traynor wrote: >> Fix minor issue of one additional retry and add >> documentation about vhost tx retries and ways to >> reduce/remove them. >> >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> >> --- >> There is a checkpatch warning that one of the libvirt >> lines in docs is a few characters too long. It is similar >> for some others in the file and I think it makes sense to >> leave it as is but can wrap if required. >> --- >> Documentation/topics/dpdk/vhost-user.rst | 31 ++++++++++++++++++++++++ >> lib/netdev-dpdk.c | 2 +- >> 2 files changed, 32 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >> index f7b4b338e..d1682fd05 100644 >> --- a/Documentation/topics/dpdk/vhost-user.rst >> +++ b/Documentation/topics/dpdk/vhost-user.rst >> @@ -76,4 +76,35 @@ mode ports require QEMU version 2.7. Ports of type vhost-user are currently >> deprecated and will be removed in a future release. >> >> +vhost tx retries >> +~~~~~~~~~~~~~~~~ >> + >> +When sending a batch of packets (max is 32) to a vhost-user or > > Maybe use the actual define name (NETDEV_MAX_BURST) since it might change? > I agree the doc could go stale if it ever changed. OTOH, I want a user to be able to read the doc part as a standalone without having to search the code. I think best to say NETDEV_MAX_BURST and add it's currently 32. If it ever changed in the code, an author or reviewer would find the comment through a search and be able to update. >> +vhost-user-client interface, it may happen that some but not all of the packets >> +in the batch are able to be sent to the guest. This is often because there is >> +not enough free descriptors in the virtqueue for all the packets to be sent. >> +In this case there will be a retry, with a default maximum of 8 occurring. If >> +at any time no packets can be sent, it may mean the guest is not accepting >> +packets, so there are no (more) retries. >> + >> +Tx Retries may be reduced or removed by some external configuration, such > > s/removed/even avoided/ ? > That sounds better, will change it. >> +as increasing the virtqueue size through the ``rx_queue_size`` parameter in >> +libvirt:: >> + >> + <interface type='vhostuser'> >> + <mac address='56:48:4f:53:54:01'/> >> + <source type='unix' path='/tmp/dpdkvhostclient0' mode='server'/> >> + <model type='virtio'/> >> + <driver name='vhost' rx_queue_size='1024' tx_queue_size='1024'/> >> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> >> + </interface> > > Do we need to worry about the qemu version? > hmm, yeah, it was definitely not always available. I will check the minimum version and add. >> + >> +The guest application will also need need to provide enough descriptors. For >> +example with ``testpmd`` the command line argument can be used:: >> + >> + --rxd=1024 --txd=1024 >> + >> +The guest should also have sufficient cores dedicated for consuming and >> +processing packets at the required rate. >> + >> .. _dpdk-vhost-user: >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 4856c56aa..0f3b9c6f4 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2353,5 +2353,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >> break; >> } >> - } while (cnt && (retries++ <= VHOST_ENQ_RETRY_NUM)); >> + } while (cnt && (retries++ < VHOST_ENQ_RETRY_NUM)); > > You're removing the packet's last hope to reach home, are you sure? :-) pop-up dialog box coming in v2 :-) thanks for review, Kevin. > fbl > >> >> rte_spinlock_unlock(&dev->tx_q[qid].tx_lock); >> -- >> 2.20.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 25.06.2019 12:32, Kevin Traynor wrote: > On 24/06/2019 22:40, Flavio Leitner wrote: >> On Fri, Jun 21, 2019 at 02:41:56PM +0100, Kevin Traynor wrote: >>> Fix minor issue of one additional retry and add >>> documentation about vhost tx retries and ways to >>> reduce/remove them. >>> >>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>> >>> --- >>> There is a checkpatch warning that one of the libvirt >>> lines in docs is a few characters too long. It is similar >>> for some others in the file and I think it makes sense to >>> leave it as is but can wrap if required. >>> --- >>> Documentation/topics/dpdk/vhost-user.rst | 31 ++++++++++++++++++++++++ >>> lib/netdev-dpdk.c | 2 +- >>> 2 files changed, 32 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>> index f7b4b338e..d1682fd05 100644 >>> --- a/Documentation/topics/dpdk/vhost-user.rst >>> +++ b/Documentation/topics/dpdk/vhost-user.rst >>> @@ -76,4 +76,35 @@ mode ports require QEMU version 2.7. Ports of type vhost-user are currently >>> deprecated and will be removed in a future release. >>> >>> +vhost tx retries >>> +~~~~~~~~~~~~~~~~ >>> + >>> +When sending a batch of packets (max is 32) to a vhost-user or >> >> Maybe use the actual define name (NETDEV_MAX_BURST) since it might change? >> > > I agree the doc could go stale if it ever changed. OTOH, I want a user > to be able to read the doc part as a standalone without having to search > the code. I think best to say NETDEV_MAX_BURST and add it's currently > 32. If it ever changed in the code, an author or reviewer would find the > comment through a search and be able to update. > >>> +vhost-user-client interface, it may happen that some but not all of the packets >>> +in the batch are able to be sent to the guest. This is often because there is >>> +not enough free descriptors in the virtqueue for all the packets to be sent. >>> +In this case there will be a retry, with a default maximum of 8 occurring. If >>> +at any time no packets can be sent, it may mean the guest is not accepting >>> +packets, so there are no (more) retries. >>> + >>> +Tx Retries may be reduced or removed by some external configuration, such >> >> s/removed/even avoided/ ? >> > > That sounds better, will change it. > >>> +as increasing the virtqueue size through the ``rx_queue_size`` parameter in >>> +libvirt:: >>> + >>> + <interface type='vhostuser'> >>> + <mac address='56:48:4f:53:54:01'/> >>> + <source type='unix' path='/tmp/dpdkvhostclient0' mode='server'/> >>> + <model type='virtio'/> >>> + <driver name='vhost' rx_queue_size='1024' tx_queue_size='1024'/> >>> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> >>> + </interface> >> >> Do we need to worry about the qemu version? >> > > hmm, yeah, it was definitely not always available. I will check the > minimum version and add. > >>> + >>> +The guest application will also need need to provide enough descriptors. For >>> +example with ``testpmd`` the command line argument can be used:: >>> + >>> + --rxd=1024 --txd=1024 >>> + >>> +The guest should also have sufficient cores dedicated for consuming and >>> +processing packets at the required rate. >>> + >>> .. _dpdk-vhost-user: >>> >>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>> index 4856c56aa..0f3b9c6f4 100644 >>> --- a/lib/netdev-dpdk.c >>> +++ b/lib/netdev-dpdk.c >>> @@ -2353,5 +2353,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >>> break; >>> } >>> - } while (cnt && (retries++ <= VHOST_ENQ_RETRY_NUM)); >>> + } while (cnt && (retries++ < VHOST_ENQ_RETRY_NUM)); >> >> You're removing the packet's last hope to reach home, are you sure? :-) > > pop-up dialog box coming in v2 :-) Maybe we could just increase VHOST_ENQ_RETRY_NUM by 1 to avoid any functional changes in this patch?
On 25/06/2019 11:17, Ilya Maximets wrote: > On 25.06.2019 12:32, Kevin Traynor wrote: >> On 24/06/2019 22:40, Flavio Leitner wrote: >>> On Fri, Jun 21, 2019 at 02:41:56PM +0100, Kevin Traynor wrote: >>>> Fix minor issue of one additional retry and add >>>> documentation about vhost tx retries and ways to >>>> reduce/remove them. >>>> >>>> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >>>> >>>> --- >>>> There is a checkpatch warning that one of the libvirt >>>> lines in docs is a few characters too long. It is similar >>>> for some others in the file and I think it makes sense to >>>> leave it as is but can wrap if required. >>>> --- >>>> Documentation/topics/dpdk/vhost-user.rst | 31 ++++++++++++++++++++++++ >>>> lib/netdev-dpdk.c | 2 +- >>>> 2 files changed, 32 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>> index f7b4b338e..d1682fd05 100644 >>>> --- a/Documentation/topics/dpdk/vhost-user.rst >>>> +++ b/Documentation/topics/dpdk/vhost-user.rst >>>> @@ -76,4 +76,35 @@ mode ports require QEMU version 2.7. Ports of type vhost-user are currently >>>> deprecated and will be removed in a future release. >>>> >>>> +vhost tx retries >>>> +~~~~~~~~~~~~~~~~ >>>> + >>>> +When sending a batch of packets (max is 32) to a vhost-user or >>> >>> Maybe use the actual define name (NETDEV_MAX_BURST) since it might change? >>> >> >> I agree the doc could go stale if it ever changed. OTOH, I want a user >> to be able to read the doc part as a standalone without having to search >> the code. I think best to say NETDEV_MAX_BURST and add it's currently >> 32. If it ever changed in the code, an author or reviewer would find the >> comment through a search and be able to update. >> >>>> +vhost-user-client interface, it may happen that some but not all of the packets >>>> +in the batch are able to be sent to the guest. This is often because there is >>>> +not enough free descriptors in the virtqueue for all the packets to be sent. >>>> +In this case there will be a retry, with a default maximum of 8 occurring. If >>>> +at any time no packets can be sent, it may mean the guest is not accepting >>>> +packets, so there are no (more) retries. >>>> + >>>> +Tx Retries may be reduced or removed by some external configuration, such >>> >>> s/removed/even avoided/ ? >>> >> >> That sounds better, will change it. >> >>>> +as increasing the virtqueue size through the ``rx_queue_size`` parameter in >>>> +libvirt:: >>>> + >>>> + <interface type='vhostuser'> >>>> + <mac address='56:48:4f:53:54:01'/> >>>> + <source type='unix' path='/tmp/dpdkvhostclient0' mode='server'/> >>>> + <model type='virtio'/> >>>> + <driver name='vhost' rx_queue_size='1024' tx_queue_size='1024'/> >>>> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> >>>> + </interface> >>> >>> Do we need to worry about the qemu version? >>> >> >> hmm, yeah, it was definitely not always available. I will check the >> minimum version and add. >> >>>> + >>>> +The guest application will also need need to provide enough descriptors. For >>>> +example with ``testpmd`` the command line argument can be used:: >>>> + >>>> + --rxd=1024 --txd=1024 >>>> + >>>> +The guest should also have sufficient cores dedicated for consuming and >>>> +processing packets at the required rate. >>>> + >>>> .. _dpdk-vhost-user: >>>> >>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >>>> index 4856c56aa..0f3b9c6f4 100644 >>>> --- a/lib/netdev-dpdk.c >>>> +++ b/lib/netdev-dpdk.c >>>> @@ -2353,5 +2353,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, >>>> break; >>>> } >>>> - } while (cnt && (retries++ <= VHOST_ENQ_RETRY_NUM)); >>>> + } while (cnt && (retries++ < VHOST_ENQ_RETRY_NUM)); >>> >>> You're removing the packet's last hope to reach home, are you sure? :-) >> >> pop-up dialog box coming in v2 :-) > > Maybe we could just increase VHOST_ENQ_RETRY_NUM by 1 to avoid any > functional changes in this patch? > Just wasn't sure if it was worth it's own patch for something so minor but I can put it in a separate patch and leave this patch to be pure docs for v2. I checked last week and it looks like the additional retry crept in at some point with another change.
diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst index f7b4b338e..d1682fd05 100644 --- a/Documentation/topics/dpdk/vhost-user.rst +++ b/Documentation/topics/dpdk/vhost-user.rst @@ -76,4 +76,35 @@ mode ports require QEMU version 2.7. Ports of type vhost-user are currently deprecated and will be removed in a future release. +vhost tx retries +~~~~~~~~~~~~~~~~ + +When sending a batch of packets (max is 32) to a vhost-user or +vhost-user-client interface, it may happen that some but not all of the packets +in the batch are able to be sent to the guest. This is often because there is +not enough free descriptors in the virtqueue for all the packets to be sent. +In this case there will be a retry, with a default maximum of 8 occurring. If +at any time no packets can be sent, it may mean the guest is not accepting +packets, so there are no (more) retries. + +Tx Retries may be reduced or removed by some external configuration, such +as increasing the virtqueue size through the ``rx_queue_size`` parameter in +libvirt:: + + <interface type='vhostuser'> + <mac address='56:48:4f:53:54:01'/> + <source type='unix' path='/tmp/dpdkvhostclient0' mode='server'/> + <model type='virtio'/> + <driver name='vhost' rx_queue_size='1024' tx_queue_size='1024'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x10' function='0x0'/> + </interface> + +The guest application will also need need to provide enough descriptors. For +example with ``testpmd`` the command line argument can be used:: + + --rxd=1024 --txd=1024 + +The guest should also have sufficient cores dedicated for consuming and +processing packets at the required rate. + .. _dpdk-vhost-user: diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 4856c56aa..0f3b9c6f4 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -2353,5 +2353,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid, break; } - } while (cnt && (retries++ <= VHOST_ENQ_RETRY_NUM)); + } while (cnt && (retries++ < VHOST_ENQ_RETRY_NUM)); rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
Fix minor issue of one additional retry and add documentation about vhost tx retries and ways to reduce/remove them. Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- There is a checkpatch warning that one of the libvirt lines in docs is a few characters too long. It is similar for some others in the file and I think it makes sense to leave it as is but can wrap if required. --- Documentation/topics/dpdk/vhost-user.rst | 31 ++++++++++++++++++++++++ lib/netdev-dpdk.c | 2 +- 2 files changed, 32 insertions(+), 1 deletion(-)