[PATCH] tap: Send frames after the first one in tap_send_frames_pasta()

newer
[PATCH v2 0/9] error logging fixes

older
[PATCH v2] pasta: propagate exit...

Stefano Brivio

13 Feb 2023 13 Feb '23

2:12 a.m.

...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 --- 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--; } } } -- 2.35.1

Show replies by date

David Gibson

13 Feb 13 Feb

3:24 a.m.

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 --- 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

...

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

Stefano Brivio

11:46 a.m.

On Mon, 13 Feb 2023 13:24:58 +1100 David Gibson 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 --- 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

...
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

David Gibson

14 Feb 14 Feb

12:39 a.m.

On Mon, Feb 13, 2023 at 11:46:09AM +0100, Stefano Brivio wrote:

...

On Mon, 13 Feb 2023 13:24:58 +1100 David Gibson 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 --- 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

...
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

833

Age (days ago)

833

Last active (days ago)

List overview

Download

3 comments

2 participants

participants (2)

David Gibson
Stefano Brivio