bnxt_en: Improve sysctl handler bnxt_dcb_list_app()

[zlei]

Summary

Prefer sbuf_new_for_sysctl() over error-prone manually managed buf.

No functional change intended.

MFC after: 1 week

[zlei]

I'm not sure if this helps 284073 but it appears that the buf is unlikely to overflow.

[markj]

sys/dev/bnxt/bnxt_en/bnxt_sysctl.c

1941–1942

We can avoid imposing an arbitrary limit by using sbuf_new_for_sysctl() here. Is there some reason you didn't use that?

[zlei]

sys/dev/bnxt/bnxt_en/bnxt_sysctl.c

1941–1942

I've ever considered sbuf_new_for_sysctl(), but it lacks the ability to pass in SBUF_NOWAIT .

Well M_WAITOK is probably OK within sysctl call, but I'd prefer to keep the behavior unchanged.

[markj]

sys/dev/bnxt/bnxt_en/bnxt_sysctl.c

1941–1942

Yes, it's always ok to use M_WAITOK in sysctl handlers, and in syscall handlers more generally, so long as the thread isn't holding non-sleepable locks.

I don't feel too strongly about it, but IMHO if you're going to improve the handler, you might as well fix this aspect of it as well.

1945–1947

If you want to report truncation as an error, the return value of sbuf_finish() should be checked.

[zlei]

Prefer sbuf_new_for_sysctl(), and calculate the buf size according to num of apps.

After a carefully calculating, with maximum apps, i.e. 128 apps, the 4096 bytes buf is still not sufficent. A hand crafted test shows that it requires about 5907 bytes. To make the buf aligned, choose 64 bytes per app.

Ideally I want a sbuf_new_for_sysctl_flag() which can pass in SBUF_AUTOEXTEND. That will be much more flexible, but probably not in this context. As that requires new KPI. User may request ERRTA fix in releng branch so just use sbuf_new_for_sysctl() right now.

[markj]

In D48496#1107088, @zlei wrote:

Prefer sbuf_new_for_sysctl(), and calculate the buf size according to num of apps.

After a carefully calculating, with maximum apps, i.e. 128 apps, the 4096 bytes buf is still not sufficent. A hand crafted test shows that it requires about 5907 bytes. To make the buf aligned, choose 64 bytes per app.

Ideally I want a sbuf_new_for_sysctl_flag() which can pass in SBUF_AUTOEXTEND. That will be much more flexible, but probably not in this context. As that requires new KPI. User may request ERRTA fix in releng branch so just use sbuf_new_for_sysctl() right now.

Note that sbuf_new_for_sysctl() creates a stream: the sbuf has a drain function, sbuf_sysctl_drain(), which uses SYSCTL_OUT to copy out the buffer, so once the buffer is full, sbuf_putc() can drain the buffer and keep going. So, the buffer size is not too important, and SBUF_AUTOEXTEND shouldn't be needed.

[zlei]

In D48496#1107102, @markj wrote:

In D48496#1107088, @zlei wrote:

Prefer sbuf_new_for_sysctl(), and calculate the buf size according to num of apps.

After a carefully calculating, with maximum apps, i.e. 128 apps, the 4096 bytes buf is still not sufficent. A hand crafted test shows that it requires about 5907 bytes. To make the buf aligned, choose 64 bytes per app.

Ideally I want a sbuf_new_for_sysctl_flag() which can pass in SBUF_AUTOEXTEND. That will be much more flexible, but probably not in this context. As that requires new KPI. User may request ERRTA fix in releng branch so just use sbuf_new_for_sysctl() right now.

Note that sbuf_new_for_sysctl() creates a stream: the sbuf has a drain function, sbuf_sysctl_drain(), which uses SYSCTL_OUT to copy out the buffer, so once the buffer is full, sbuf_putc() can drain the buffer and keep going. So, the buffer size is not too important, and SBUF_AUTOEXTEND shouldn't be needed.

Ahh, you're right ! I only looked into sbuf_finish() but not sbuf_printf(). So a small buffer is OK for sbuf_new_for_sysctl(). I'm going to choose a buffer size, say 128. Does that look good to you ?

[markj]

In D48496#1107169, @zlei wrote:

In D48496#1107102, @markj wrote:

Note that sbuf_new_for_sysctl() creates a stream: the sbuf has a drain function, sbuf_sysctl_drain(), which uses SYSCTL_OUT to copy out the buffer, so once the buffer is full, sbuf_putc() can drain the buffer and keep going. So, the buffer size is not too important, and SBUF_AUTOEXTEND shouldn't be needed.

Ahh, you're right ! I only looked into sbuf_finish() but not sbuf_printf(). So a small buffer is OK for sbuf_new_for_sysctl(). I'm going to choose a buffer size, say 128. Does that look good to you ?

I think that's fine, the buffer size is mostly arbitrary in this case. 128 bytes should work.

[zlei]

Use a small buffer size 128.