Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: Memory spike reduction for sh cmds at scale #16498

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions lib/vty.c
Original file line number Diff line number Diff line change
Expand Up @@ -345,8 +345,17 @@ int vty_out(struct vty *vty, const char *format, ...)
case VTY_SHELL_SERV:
case VTY_FILE:
default:
vty->vty_buf_size_written += strlen(filtered);
/* print without crlf replacement */
buffer_put(vty->obuf, (uint8_t *)filtered, strlen(filtered));
/* For every chunk of memory, we invoke vtysh_flush where we
* put the data of collective vty->obuf Linked List items on the
* socket and free the vty->obuf data.
*/
if (vty->vty_buf_size_written >= MAX_INTERMEDIATE_FLUSH) {
vty->vty_buf_size_written = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still unclear about the meaning of this new variable.
it's set to zero here, even if nothing was written (or less than the amount of data pending), so it no longer represents the amount of data buffered?
it's set to zero (again) on a complete send.
what is it supposed to represent - it's not the "buf_size_written", right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is, that we keep accumulating data (few vty_outs() calls) until we hit 32KB and once we hit the 32KB limit,

  • we call vtysh_flush where the data(32KB) is written into the socket
  • reset vty->vty_buf_size_written to 0 so we again accumulate another 32KB.

vty_buf_size_written is essentially to tell me how much memory is accumulated. if renaming it makes life easy, would vty_buf_size_accumulated work?

vtysh_flush(vty);
}
break;
}

Expand Down Expand Up @@ -2141,6 +2150,21 @@ static void vtysh_accept(struct event *thread)
close(sock);
return;
}
/* Setting the SND buffer size to 16MB to increase the buffer size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't embed the current value in the comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

* the socket can hold before sending it to VTY shell
*/
uint32_t sndbufsize = SEND_BUF_MAX;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please declare the locals at the top, follow the existing style

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

int ret;

ret = setsockopt(sock, SOL_SOCKET, SO_SNDBUF, (char *)&sndbufsize,
sizeof(sndbufsize));
if (ret < 0) {
flog_err(EC_LIB_SOCKET,
"Cannot set socket %d send buffer size, %s", sock,
safe_strerror(errno));
close(sock);
return;
}
set_cloexec(sock);

#ifdef VTYSH_DEBUG
Expand Down Expand Up @@ -2227,6 +2251,7 @@ static int vtysh_flush(struct vty *vty)
vty_close(vty);
return -1;
case BUFFER_EMPTY:
vty->vty_buf_size_written = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the counter is only reset if obuf is empty? if the write partially succeeds, we'll stay in a "flush" loop with every vty_out() call - is that ... what you want?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there was an extra 2-3 MB used with reset when it hits the 128 KB and we will still have our 25-30 % memory reduction, so I changed it and it will reset in both the places and we won't be in flush loop for every vty_out call and it calls only after we reach the chunk is 128KB again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the buffer is empty, should we cancel a "vtysh_write" event?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I understood your question right. You are suggesting we should cancel the vtysh_write event when the buffer is empty?
In the existing code vtysh_flush is called at the end of command execution (considering outputs less than 128KB also). After that vty_close is called which will cancel the vtysh_write event since it is end of command execution. I thought it is better to leave the existing code as is and did not include intermediate cancel and starting of vtysh_write event.

break;
}
return 0;
Expand Down
7 changes: 7 additions & 0 deletions lib/vty.h
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ struct vty {
uintptr_t mgmt_req_pending_data;
bool mgmt_locked_candidate_ds;
bool mgmt_locked_running_ds;
uint64_t vty_buf_size_written;
};

static inline void vty_push_context(struct vty *vty, int node, uint64_t id)
Expand Down Expand Up @@ -338,6 +339,12 @@ struct vty_arg {
/* Vty read buffer size. */
#define VTY_READ_BUFSIZ 512

/* Vty max send buffer size (4096 * 4096). */
#define SEND_BUF_MAX 16777216

/* Vty flush intermediate size (32 * 4096). */
#define MAX_INTERMEDIATE_FLUSH 131072

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to put these in the public vty.h header, please follow the naming pattern and call them VTY_xxx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

/* Directory separator. */
#ifndef DIRECTORY_SEP
#define DIRECTORY_SEP '/'
Expand Down
16 changes: 16 additions & 0 deletions vtysh/vtysh.c
Original file line number Diff line number Diff line change
Expand Up @@ -4721,6 +4721,22 @@ static int vtysh_connect(struct vtysh_client *vclient)
close(sock);
return -1;
}
/* Setting the RCV buffer size to 16MB to increase the buffer size
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't embed the value in the comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

* the socket can hold after receving from other process
*/
uint32_t rcvbufsize = RCV_BUF_MAX;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please declare the locals at the top, follow the existing style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do


ret = setsockopt(sock, SOL_SOCKET, SO_RCVBUF, (char *)&rcvbufsize,
sizeof(rcvbufsize));
if (ret < 0) {
#ifdef DEBUG
skanchisa marked this conversation as resolved.
Show resolved Hide resolved
fprintf(stderr, "Cannot set socket %d rcv buffer size, %s\n",
sock, safe_strerror(errno));
#endif /* DEBUG */
close(sock);
return -1;
}

vclient->fd = sock;

return 0;
Expand Down
1 change: 1 addition & 0 deletions vtysh/vtysh.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ extern struct event_loop *master;
#define VTYSH_PATHD 0x80000
#define VTYSH_PIM6D 0x100000
#define VTYSH_MGMTD 0x200000
#define RCV_BUF_MAX 16777216
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please follow the naming convention used VTYSH_xxx

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do


#define VTYSH_WAS_ACTIVE (-2)

Expand Down
Loading