[v4,01/12] pkt-line: eliminate the need for static buffer in packet_write_gently()

Commit Message

Jeff Hostetler Feb. 17, 2021, 9:48 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Teach `packet_write_gently()` to write the pkt-line header and the actual
buffer in 2 separate calls to `write_in_full()` and avoid the need for a
static buffer, thread-safe scratch space, or an excessively large stack
buffer.

Change the API of `write_packetized_from_fd()` to accept a scratch space
argument from its caller to avoid similar issues here.

These changes are intended to make it easier to use pkt-line routines in
a multi-threaded context with multiple concurrent writers writing to
different streams.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 convert.c  |  7 ++++---
 pkt-line.c | 28 +++++++++++++++++++---------
 pkt-line.h | 12 +++++++++---
 3 files changed, 32 insertions(+), 15 deletions(-)

Comments

Jeff King Feb. 26, 2021, 7:21 a.m. UTC | #1

On Wed, Feb 17, 2021 at 09:48:37PM +0000, Jeff Hostetler via GitGitGadget wrote:

> Change the API of `write_packetized_from_fd()` to accept a scratch space
> argument from its caller to avoid similar issues here.

OK, but...

> diff --git a/convert.c b/convert.c
> index ee360c2f07ce..41012c2d301c 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -883,9 +883,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>  	if (err)
>  		goto done;
>  
> -	if (fd >= 0)
> -		err = write_packetized_from_fd(fd, process->in);
> -	else
> +	if (fd >= 0) {
> +		struct packet_scratch_space scratch;
> +		err = write_packetized_from_fd(fd, process->in, &scratch);
> +	} else
>  		err = write_packetized_from_buf(src, len, process->in);

Isn't this just putting the buffer onto the stack anyway? Your
scratch_space struct is really just a big array. You'd want to make
it static here, but then we haven't really solved anything. :)

I think instead that:

> -int write_packetized_from_fd(int fd_in, int fd_out)
> +int write_packetized_from_fd(int fd_in, int fd_out,
> +			     struct packet_scratch_space *scratch)
>  {
> -	static char buf[LARGE_PACKET_DATA_MAX];
>  	int err = 0;
>  	ssize_t bytes_to_write;
>  
>  	while (!err) {
> -		bytes_to_write = xread(fd_in, buf, sizeof(buf));
> +		bytes_to_write = xread(fd_in, scratch->buffer,
> +				       sizeof(scratch->buffer));
>  		if (bytes_to_write < 0)
>  			return COPY_READ_ERROR;
>  		if (bytes_to_write == 0)
>  			break;
> -		err = packet_write_gently(fd_out, buf, bytes_to_write);
> +		err = packet_write_gently(fd_out, scratch->buffer,
> +					  bytes_to_write);
>  	}

...just heap-allocating the buffer in this function would be fine. It's
one malloc for the whole sequence of pktlines, which is unlikely to be a
problem.

-Peff

Jeff Hostetler Feb. 26, 2021, 7:52 p.m. UTC | #2

On 2/26/21 2:21 AM, Jeff King wrote:
> On Wed, Feb 17, 2021 at 09:48:37PM +0000, Jeff Hostetler via GitGitGadget wrote:
> 
>> Change the API of `write_packetized_from_fd()` to accept a scratch space
>> argument from its caller to avoid similar issues here.
> 
> OK, but...
> 
>> diff --git a/convert.c b/convert.c
>> index ee360c2f07ce..41012c2d301c 100644
>> --- a/convert.c
>> +++ b/convert.c
>> @@ -883,9 +883,10 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len
>>   	if (err)
>>   		goto done;
>>   
>> -	if (fd >= 0)
>> -		err = write_packetized_from_fd(fd, process->in);
>> -	else
>> +	if (fd >= 0) {
>> +		struct packet_scratch_space scratch;
>> +		err = write_packetized_from_fd(fd, process->in, &scratch);
>> +	} else
>>   		err = write_packetized_from_buf(src, len, process->in);
> 
> Isn't this just putting the buffer onto the stack anyway? Your
> scratch_space struct is really just a big array. You'd want to make
> it static here, but then we haven't really solved anything. :)

Yeah, I was letting the caller decide how to provide the buffer.
They could put it on the stack or allocate it once across a whole
set of files or use a static buffer -- the caller has context for
what works best that we don't have here.  For example, the caller
may know that is not in threaded code at all, but we cannot assume
that here.

> 
> I think instead that:
> 
>> -int write_packetized_from_fd(int fd_in, int fd_out)
>> +int write_packetized_from_fd(int fd_in, int fd_out,
>> +			     struct packet_scratch_space *scratch)
>>   {
>> -	static char buf[LARGE_PACKET_DATA_MAX];
>>   	int err = 0;
>>   	ssize_t bytes_to_write;
>>   
>>   	while (!err) {
>> -		bytes_to_write = xread(fd_in, buf, sizeof(buf));
>> +		bytes_to_write = xread(fd_in, scratch->buffer,
>> +				       sizeof(scratch->buffer));
>>   		if (bytes_to_write < 0)
>>   			return COPY_READ_ERROR;
>>   		if (bytes_to_write == 0)
>>   			break;
>> -		err = packet_write_gently(fd_out, buf, bytes_to_write);
>> +		err = packet_write_gently(fd_out, scratch->buffer,
>> +					  bytes_to_write);
>>   	}
> 
> ...just heap-allocating the buffer in this function would be fine. It's
> one malloc for the whole sequence of pktlines, which is unlikely to be a
> problem.

Right, I think it would be fine to malloc it here, but I didn't
want to assume that everyone would think that.

I'll change it.

Thanks
Jeff

Jeff King Feb. 26, 2021, 8:43 p.m. UTC | #3

On Fri, Feb 26, 2021 at 02:52:22PM -0500, Jeff Hostetler wrote:

> > > -	if (fd >= 0)
> > > -		err = write_packetized_from_fd(fd, process->in);
> > > -	else
> > > +	if (fd >= 0) {
> > > +		struct packet_scratch_space scratch;
> > > +		err = write_packetized_from_fd(fd, process->in, &scratch);
> > > +	} else
> > >   		err = write_packetized_from_buf(src, len, process->in);
> > 
> > Isn't this just putting the buffer onto the stack anyway? Your
> > scratch_space struct is really just a big array. You'd want to make
> > it static here, but then we haven't really solved anything. :)
> 
> Yeah, I was letting the caller decide how to provide the buffer.
> They could put it on the stack or allocate it once across a whole
> set of files or use a static buffer -- the caller has context for
> what works best that we don't have here.  For example, the caller
> may know that is not in threaded code at all, but we cannot assume
> that here.

Yeah, I think it's successfully pushed the problem up to the caller. But
it introduced a _new_ problem in putting the large buffer on the stack.
So if this were "static struct packet_scratch_space scratch", I think
we'd be OK.

And perhaps that would meet your needs (if you just need to call
write_packed_from_fd() in a thread, and not this other caller).

But I do think the heap approach is nice in that it keeps the interface
clean, and I think the performance should be comparable.

> Right, I think it would be fine to malloc it here, but I didn't
> want to assume that everyone would think that.
> 
> I'll change it.

Thanks. :)

-Peff

Junio C Hamano March 3, 2021, 7:38 p.m. UTC | #4

Jeff Hostetler <[e-mail]> writes:

> Right, I think it would be fine to malloc it here, but I didn't
> want to assume that everyone would think that.
>
> I'll change it.

I agree with both of you that the code is unnice in its stack usage
and we want fix with malloc(), or something like that, but sorry, I
think I merged this round by mistake to 'next'.

As we won't be merging the topic to the upcoming release anyway, I
am willing to revert the merge to 'next' and requeue an updated one,
when it appears (I am also OK to see an incremental update, "oops,
no, we realize we don't want to have it on the stack" fix-up, if
this is the only glitch in the series that need to be fixed).

Thanks.

Jeff Hostetler March 4, 2021, 1:29 p.m. UTC | #5

On 3/3/21 2:38 PM, Junio C Hamano wrote:
> Jeff Hostetler <[e-mail]> writes:
> 
>> Right, I think it would be fine to malloc it here, but I didn't
>> want to assume that everyone would think that.
>>
>> I'll change it.
> 
> I agree with both of you that the code is unnice in its stack usage
> and we want fix with malloc(), or something like that, but sorry, I
> think I merged this round by mistake to 'next'.
> 
> As we won't be merging the topic to the upcoming release anyway, I
> am willing to revert the merge to 'next' and requeue an updated one,
> when it appears (I am also OK to see an incremental update, "oops,
> no, we realize we don't want to have it on the stack" fix-up, if
> this is the only glitch in the series that need to be fixed).
> 
> Thanks.
> 

I'm preparing a follow-on patch series to address Peff's comments
from Friday/Monday and yours from yesterday.  I thought I'd send
it as a set of new changes to sit on top of what we have in "next"
if that would make things easier for you.

After the upcoming release we can talk about whether it would be
better for me to smash together the 2 series or not.

Jeff

Junio C Hamano March 4, 2021, 8:26 p.m. UTC | #6

Jeff Hostetler <[e-mail]> writes:

> On 3/3/21 2:38 PM, Junio C Hamano wrote:
>
>> I agree with both of you that the code is unnice in its stack usage
>> and we want fix with malloc(), or something like that, but sorry, I
>> think I merged this round by mistake to 'next'.
>> As we won't be merging the topic to the upcoming release anyway, I
>> am willing to revert the merge to 'next' and requeue an updated one,
>> when it appears (I am also OK to see an incremental update, "oops,
>> no, we realize we don't want to have it on the stack" fix-up, if
>> this is the only glitch in the series that need to be fixed).
>
> I'm preparing a follow-on patch series to address Peff's comments
> from Friday/Monday and yours from yesterday.  I thought I'd send
> it as a set of new changes to sit on top of what we have in "next"
> if that would make things easier for you.

Yeah, that is OK, too.  Sorry for the mistake of merging it too
early.

Patch

diff --git a/convert.c b/convert.c
index ee360c2f07ce..41012c2d301c 100644
--- a/convert.c
+++ b/convert.c
@@ -883,9 +883,10 @@  static int apply_multi_file_filter(const char *path, const char *src, size_t len
 	if (err)
 		goto done;
 
-	if (fd >= 0)
-		err = write_packetized_from_fd(fd, process->in);
-	else
+	if (fd >= 0) {
+		struct packet_scratch_space scratch;
+		err = write_packetized_from_fd(fd, process->in, &scratch);
+	} else
 		err = write_packetized_from_buf(src, len, process->in);
 	if (err)
 		goto done;
diff --git a/pkt-line.c b/pkt-line.c
index d633005ef746..4cff2f7a68a5 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -196,17 +196,25 @@  int packet_write_fmt_gently(int fd, const char *fmt, ...)
 
 static int packet_write_gently(const int fd_out, const char *buf, size_t size)
 {
-	static char packet_write_buffer[LARGE_PACKET_MAX];
+	char header[4];
 	size_t packet_size;
 
-	if (size > sizeof(packet_write_buffer) - 4)
+	if (size > LARGE_PACKET_DATA_MAX)
 		return error(_("packet write failed - data exceeds max packet size"));
 
 	packet_trace(buf, size, 1);
 	packet_size = size + 4;
-	set_packet_header(packet_write_buffer, packet_size);
-	memcpy(packet_write_buffer + 4, buf, size);
-	if (write_in_full(fd_out, packet_write_buffer, packet_size) < 0)
+
+	set_packet_header(header, packet_size);
+
+	/*
+	 * Write the header and the buffer in 2 parts so that we do not need
+	 * to allocate a buffer or rely on a static buffer.  This avoids perf
+	 * and multi-threading issues.
+	 */
+
+	if (write_in_full(fd_out, header, 4) < 0 ||
+	    write_in_full(fd_out, buf, size) < 0)
 		return error(_("packet write failed"));
 	return 0;
 }
@@ -242,19 +250,21 @@  void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)
 	packet_trace(data, len, 1);
 }
 
-int write_packetized_from_fd(int fd_in, int fd_out)
+int write_packetized_from_fd(int fd_in, int fd_out,
+			     struct packet_scratch_space *scratch)
 {
-	static char buf[LARGE_PACKET_DATA_MAX];
 	int err = 0;
 	ssize_t bytes_to_write;
 
 	while (!err) {
-		bytes_to_write = xread(fd_in, buf, sizeof(buf));
+		bytes_to_write = xread(fd_in, scratch->buffer,
+				       sizeof(scratch->buffer));
 		if (bytes_to_write < 0)
 			return COPY_READ_ERROR;
 		if (bytes_to_write == 0)
 			break;
-		err = packet_write_gently(fd_out, buf, bytes_to_write);
+		err = packet_write_gently(fd_out, scratch->buffer,
+					  bytes_to_write);
 	}
 	if (!err)
 		err = packet_flush_gently(fd_out);
diff --git a/pkt-line.h b/pkt-line.h
index 8c90daa59ef0..c0722aefe638 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -5,6 +5,13 @@ 
 #include "strbuf.h"
 #include "sideband.h"
 
+#define LARGE_PACKET_MAX 65520
+#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
+struct packet_scratch_space {
+	char buffer[LARGE_PACKET_DATA_MAX]; /* does not include header bytes */
+};
+
 /*
  * Write a packetized stream, where each line is preceded by
  * its length (including the header) as a 4-byte hex number.
@@ -32,7 +39,7 @@  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f
 void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);
 int packet_flush_gently(int fd);
 int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
-int write_packetized_from_fd(int fd_in, int fd_out);
+int write_packetized_from_fd(int fd_in, int fd_out, struct packet_scratch_space *scratch);
 int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
 
 /*
@@ -213,8 +220,7 @@  enum packet_read_status packet_reader_read(struct packet_reader *reader);
 enum packet_read_status packet_reader_peek(struct packet_reader *reader);
 
 #define DEFAULT_PACKET_MAX 1000
-#define LARGE_PACKET_MAX 65520
-#define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)
+
 extern char packet_buffer[LARGE_PACKET_MAX];
 
 struct packet_writer {