[01/10] pkt-line: use stack rather than static buffer in packet_write_gently()

Commit Message

Jeff Hostetler Jan. 12, 2021, 3:31 p.m. UTC

From: Jeff Hostetler <[e-mail]>

Teach packet_write_gently() to use a stack buffer rather than a static
buffer when composing the packet line message.  This helps get us ready
for threaded operations.

Signed-off-by: Jeff Hostetler <[e-mail]>
---
 pkt-line.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jeff King Jan. 13, 2021, 1:29 p.m. UTC | #1

On Tue, Jan 12, 2021 at 03:31:23PM +0000, Jeff Hostetler via GitGitGadget wrote:

> Teach packet_write_gently() to use a stack buffer rather than a static
> buffer when composing the packet line message.  This helps get us ready
> for threaded operations.

Sounds like a good goal, but...

>  static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>  {
> -	static char packet_write_buffer[LARGE_PACKET_MAX];
> +	char packet_write_buffer[LARGE_PACKET_MAX];
>  	size_t packet_size;

64k is awfully big for the stack, especially if you are thinking about
having threads. I know we've run into issues around that size before
(though I don't offhand recall whether there was any recursion
involved).

We might need to use thread-local storage here. Heap would also
obviously work, but I don't think we'd want a new allocation per write
(or maybe it wouldn't matter; we're making a syscall, so a malloc() may
not be that big a deal in terms of performance).

-Peff

Jeff Hostetler Jan. 25, 2021, 7:34 p.m. UTC | #2

On 1/13/21 8:29 AM, Jeff King wrote:
> On Tue, Jan 12, 2021 at 03:31:23PM +0000, Jeff Hostetler via GitGitGadget wrote:
> 
>> Teach packet_write_gently() to use a stack buffer rather than a static
>> buffer when composing the packet line message.  This helps get us ready
>> for threaded operations.
> 
> Sounds like a good goal, but...
> 
>>   static int packet_write_gently(const int fd_out, const char *buf, size_t size)
>>   {
>> -	static char packet_write_buffer[LARGE_PACKET_MAX];
>> +	char packet_write_buffer[LARGE_PACKET_MAX];
>>   	size_t packet_size;
> 
> 64k is awfully big for the stack, especially if you are thinking about
> having threads. I know we've run into issues around that size before
> (though I don't offhand recall whether there was any recursion
> involved).
> 
> We might need to use thread-local storage here. Heap would also
> obviously work, but I don't think we'd want a new allocation per write
> (or maybe it wouldn't matter; we're making a syscall, so a malloc() may
> not be that big a deal in terms of performance).
> 
> -Peff
> 

Good point.

I'll look at the callers and see if I can do something safer.

Jeeff

Patch

diff --git a/pkt-line.c b/pkt-line.c
index d633005ef74..98439a2fed0 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -196,7 +196,7 @@  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 packet_write_buffer[LARGE_PACKET_MAX];
 	size_t packet_size;
 
 	if (size > sizeof(packet_write_buffer) - 4)