On Tue, Jun 22, 2021 at 5:22 PM Mark Michelson <mmich...@redhat.com> wrote:
>
> On 6/22/21 1:14 PM, Dan Williams wrote:
> > On Fri, 2021-06-18 at 21:49 +0200, Dumitru Ceara wrote:
> >> On 6/4/21 10:00 PM, Dan Williams wrote:
> >>> Inspried by:
> >>>
> >>> 3b6362d64e86b northd: Avoid memory reallocation while building lb
> >>> rules.
> >>>
> >>> Signed-off-by: Dan Williams <d...@redhat.com>
> >>> ---
> >>> NOTE: this is driven by visual inspection not perf data. But it
> >>> shouldn't be worse than current code and should be better for
> >>> large numbers of ACLs I think.
> >>
> >> The changes look OK to me.
> >>
> >> Acked-by: Dumitru Ceara <dce...@redhat.com>
> >>
> >> However, I wonder how many such optimizations we can implement
> >> without
> >> affecting maintainability. Mark suggested an approach [0].
> >
> > I'm happy to drop my patch in favor of Mark's. I think mine is a subset
> > of his.
> >
> > Dan
>
> Funny because I'm not even 100% behind my own approach.
Thanks Dan, Dumitru and Mark. I applied this patch to the main branch.
Numan
>
> >
> >>
> >> CC-ing Ilya too, maybe he has some more suggestions, maybe there's a
> >> way
> >> to better use the OVS dynamic strings.
>
> I did some brainstorming and came up with a test program:
>
> #include <stdio.h>
> #include <stdarg.h>
>
> static void
> my_crazy_printf(char *fmt1, char *fmt2, ...)
> {
> va_list ap;
>
> va_start(ap, fmt2);
> vprintf(fmt1, ap);
>
> va_list aq;
> va_copy(aq, ap);
>
> vprintf(fmt2, aq);
>
> va_end(aq);
> va_end(ap);
> }
>
> int main(void)
> {
> my_crazy_printf("%s=%d\n", "%s=%d\n", "howdy", 4, "byebye", 3);
> return 0;
> }
>
> On my system, the output is:
> howdy=4
> byebye=3
>
> I came up with this as a test to see how feasible it is to have two
> format strings in the same parameter list. The idea here is to translate
> that into something like this:
>
> /* I've omitted unimportant parameters */
> static void
> ovn_lflow_add(match_fmt, actions_fmt, ...)
> {
> static struct ds match = DS_EMPTY_INITIALIZER;
> static struct ds actions = DS_EMPTY_INITIALIZER;
>
> struct va_list ap;
> va_start(ap, actions_fmt);
> ds_clear(&match);
> ds_put_format_valist(&match, match_fmt, ap);
>
> struct va_list aq;
> va_copy(aq, ap);
> ds_clear(&actions);
> ds_put_format_valist(&match, actions_fmt, aq);
>
> va_end(aq);
> va_end(ap);
>
> /* The rest of the function */
> }
>
> With this, the dynamic string handling is done entirely within
> ovn_lflow_add(), meaning that the same buffers are reused. The problem
> (aside from the fact that it's weird), is that ds_put_format_valist()
> performs its own va_copy() operation of the passed-in va_list. This
> means that the two ds_put_format_valist() operations operate on
> identical va_lists. Pursuing this problem any further means essentially
> re-implementing dynamic strings to allow for this unorthodox usage. It
> feels like a dead end to me.
>
> >>
> >> Regards,
> >> Dumitru
> >>
> >> [0]
> >> https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384043.html
> >>
> >
> >
>
> [1] Test code:
>
> If you run this code, then the output is:
> howdy=4
> byebye=3
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
