Hi Alexander,
On Fri, Aug 11, 2023 at 02:08:37PM +0000, Stephan, Alexander wrote:
> Hi Willy,
>
> Thanks for the nice, detailed feedback.
> Overall, I agree with all of your listed points, so no need for further
> discussions. ?
> I will hopefully send the separated patches at the beginning of next week.
OK! No rush anyway, such changes having a low impact can be merged late
in the cycle if needed, so take your time.
> Just a comment and two small questions to make sure I got things correct:
>
> > As such I'd like them to be renamed to remove this confusion.
> > Maybe just call them HA_PP2_* ?
>
> Yes, such a prefix will clean it up quite nicely IMO. Will add that to the
> first patch of the series.
Thanks.
> > [...]
> > It may even encourage us to create
> > smaller pools if ever deemed really useful (e.g. a 4- or 8- byte
> > load balancing hash key for end-to-end consistency would only
> > take a total of 32 or 40, malloc included).
>
> Just to make sure: Right now, we don't want to optimize further for small
> TLVs other than removing the second pool for the values and using the new
> struct with the VLA to reduce the overhead.
> For normal use, a pool with 160 B could be used now to accommodate for the
> new conn_tlv_list struct and 128 B TLVs (e.g., UNIQUE_ID)?
> For the authority type, it would then be a 255 + 32 B sized pool? Maybe that
> could be used for the value range 128 <= x <= 255, and then, for > 255,
> malloc?
Yes I think that will do the job (more like 128 < x < 256 BTW). I think
you'd rather just focus on the type size (like you did) and not on the
type itself, so that pools will automatically fit (i.e. have
128+sizeof(conn_tlv_list) and 256+sizeof() and the rest is for malloc().
> Other, smaller pools are future work?
Yes, as I'm not sure they're really that much used, and it will be easy
to create smaller pools if we figure they're needed.
> > struct conn_tlv_list {
> > struct list list;
> > unsigned short len; // 65535 should be more than enough!
> > unsigned char type;
> > char contents[0];
> > };
>
> I am also a bit confused about the second struct variant of this. IMO this is
> the optimal struct layout in regards to size, that I would like to use.
> What is the other struct for, where `len` and `type` are switched?
Ah, at first I didn't know what you were talking about, I remember having
added the type while writing the message, it's just that I poorly pasted
it the second time :-) It's better to keep it as above, where types are
better aligned and leave no hole. Hmmm BTW the struct will be padded, so
you should use offsetof(conn_tlv_list, contents) rather than
sizeof(conn_tlv_list) for the size calculations. Or you can mark the struct
with __attribute__((packed)), it will do the job as well. It's up to you.
> Thanks again for your efforts, it's really interesting for me to work on
> HAProxy.
You're welcome, do not hesitate to send any question you may have.
Cheers,
Willy
