* [PATCH] tap: Send frames after the first one in tap_send_frames_pasta()
@ 2023-02-13 1:12 Stefano Brivio
2023-02-13 2:24 ` David Gibson
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Brivio @ 2023-02-13 1:12 UTC (permalink / raw)
To: passt-dev; +Cc: David Gibson
...instead of repeatedly sending out the first one in iov.
Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
I just applied this, to unblock a series by David which was pending
for way too long. The commit reference in Fixes: refers to a commit
from said series which I'm pushing out together with this patch.
Posting anyway for reviews.
tap.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tap.c b/tap.c
index af9bc15..716d887 100644
--- a/tap.c
+++ b/tap.c
@@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c,
{
size_t i;
- for (i = 0; i < n; i++) {
+ for (i = 0; i < n; i++, iov++) {
if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) {
debug("tap write: %s", strerror(errno));
if (errno != EAGAIN && errno != EWOULDBLOCK)
tap_handler(c, c->fd_tap, EPOLLERR, NULL);
i--;
+ iov--;
}
}
}
--
@@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c,
{
size_t i;
- for (i = 0; i < n; i++) {
+ for (i = 0; i < n; i++, iov++) {
if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) {
debug("tap write: %s", strerror(errno));
if (errno != EAGAIN && errno != EWOULDBLOCK)
tap_handler(c, c->fd_tap, EPOLLERR, NULL);
i--;
+ iov--;
}
}
}
--
2.35.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] tap: Send frames after the first one in tap_send_frames_pasta()
2023-02-13 1:12 [PATCH] tap: Send frames after the first one in tap_send_frames_pasta() Stefano Brivio
@ 2023-02-13 2:24 ` David Gibson
2023-02-13 10:46 ` Stefano Brivio
0 siblings, 1 reply; 4+ messages in thread
From: David Gibson @ 2023-02-13 2:24 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]
On Mon, Feb 13, 2023 at 02:12:11AM +0100, Stefano Brivio wrote:
> ...instead of repeatedly sending out the first one in iov.
>
> Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> I just applied this, to unblock a series by David which was pending
> for way too long. The commit reference in Fixes: refers to a commit
> from said series which I'm pushing out together with this patch.
Huh... how did this ever work even slightly. From that point of view,
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Posting anyway for reviews.
That said..
>
> tap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tap.c b/tap.c
> index af9bc15..716d887 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c,
> {
> size_t i;
>
> - for (i = 0; i < n; i++) {
> + for (i = 0; i < n; i++, iov++) {
I quite dislike having multiple "counters" that need to be updated for
each loop iteration (manual strength reduction. It's really easy to
make a mistake in later changes and let the two values get out of sync
- which is exactly what I did with the earlier change that introduced
this bug.
W.r.t. performance, I generally trust the compiler's automatic
strength reduction to have a better idea of whether it will be worth
it or not than my own guess.
> if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) {
So, my *intention* on the older patch was to replace 'iov->' above
with 'iov[i].'
> debug("tap write: %s", strerror(errno));
> if (errno != EAGAIN && errno != EWOULDBLOCK)
> tap_handler(c, c->fd_tap, EPOLLERR, NULL);
> i--;
> + iov--;
> }
> }
> }
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tap: Send frames after the first one in tap_send_frames_pasta()
2023-02-13 2:24 ` David Gibson
@ 2023-02-13 10:46 ` Stefano Brivio
2023-02-13 23:39 ` David Gibson
0 siblings, 1 reply; 4+ messages in thread
From: Stefano Brivio @ 2023-02-13 10:46 UTC (permalink / raw)
To: David Gibson; +Cc: passt-dev
On Mon, 13 Feb 2023 13:24:58 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:
> On Mon, Feb 13, 2023 at 02:12:11AM +0100, Stefano Brivio wrote:
> > ...instead of repeatedly sending out the first one in iov.
> >
> > Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together")
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> > I just applied this, to unblock a series by David which was pending
> > for way too long. The commit reference in Fixes: refers to a commit
> > from said series which I'm pushing out together with this patch.
>
> Huh... how did this ever work even slightly. From that point of view,
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> > Posting anyway for reviews.
>
> That said..
>
> >
> > tap.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tap.c b/tap.c
> > index af9bc15..716d887 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c,
> > {
> > size_t i;
> >
> > - for (i = 0; i < n; i++) {
> > + for (i = 0; i < n; i++, iov++) {
>
> I quite dislike having multiple "counters" that need to be updated for
> each loop iteration (manual strength reduction. It's really easy to
> make a mistake in later changes and let the two values get out of sync
> - which is exactly what I did with the earlier change that introduced
> this bug.
Um, yes. I try, whenever possible, to use just one "iterator", which
would be iov, but the price of doing that "cleanly" here is wasting a
struct iovec just to have a zero iov_len at the end, which makes little
sense.
> W.r.t. performance, I generally trust the compiler's automatic
> strength reduction to have a better idea of whether it will be worth
> it or not than my own guess.
>
> > if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) {
>
> So, my *intention* on the older patch was to replace 'iov->' above
> with 'iov[i].'
That would also be consistent with tap_send_frames_passt(), so sure,
let's change it. I can submit a patch too.
--
Stefano
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tap: Send frames after the first one in tap_send_frames_pasta()
2023-02-13 10:46 ` Stefano Brivio
@ 2023-02-13 23:39 ` David Gibson
0 siblings, 0 replies; 4+ messages in thread
From: David Gibson @ 2023-02-13 23:39 UTC (permalink / raw)
To: Stefano Brivio; +Cc: passt-dev
[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]
On Mon, Feb 13, 2023 at 11:46:09AM +0100, Stefano Brivio wrote:
> On Mon, 13 Feb 2023 13:24:58 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Mon, Feb 13, 2023 at 02:12:11AM +0100, Stefano Brivio wrote:
> > > ...instead of repeatedly sending out the first one in iov.
> > >
> > > Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together")
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > > ---
> > > I just applied this, to unblock a series by David which was pending
> > > for way too long. The commit reference in Fixes: refers to a commit
> > > from said series which I'm pushing out together with this patch.
> >
> > Huh... how did this ever work even slightly. From that point of view,
> >
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >
> > > Posting anyway for reviews.
> >
> > That said..
> >
> > >
> > > tap.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tap.c b/tap.c
> > > index af9bc15..716d887 100644
> > > --- a/tap.c
> > > +++ b/tap.c
> > > @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c,
> > > {
> > > size_t i;
> > >
> > > - for (i = 0; i < n; i++) {
> > > + for (i = 0; i < n; i++, iov++) {
> >
> > I quite dislike having multiple "counters" that need to be updated for
> > each loop iteration (manual strength reduction. It's really easy to
> > make a mistake in later changes and let the two values get out of sync
> > - which is exactly what I did with the earlier change that introduced
> > this bug.
>
> Um, yes. I try, whenever possible, to use just one "iterator", which
> would be iov, but the price of doing that "cleanly" here is wasting a
> struct iovec just to have a zero iov_len at the end, which makes little
> sense.
Right.. I mean it's nice when you can use the pointer/object itself as
the iterator. But in C, its pretty common for that to get awkward, so
I was conciously switching these from the iterator being 'iov' to the
iterator being 'i'.
> > W.r.t. performance, I generally trust the compiler's automatic
> > strength reduction to have a better idea of whether it will be worth
> > it or not than my own guess.
> >
> > > if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) {
> >
> > So, my *intention* on the older patch was to replace 'iov->' above
> > with 'iov[i].'
>
> That would also be consistent with tap_send_frames_passt(), so sure,
> let's change it. I can submit a patch too.
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-13 23:42 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-13 1:12 [PATCH] tap: Send frames after the first one in tap_send_frames_pasta() Stefano Brivio
2023-02-13 2:24 ` David Gibson
2023-02-13 10:46 ` Stefano Brivio
2023-02-13 23:39 ` David Gibson
Code repositories for project(s) associated with this public inbox
https://passt.top/passt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).