Re: [ovs-dev] [PATCH ovn] northd: avoid memory reallocation while building ACL and QoS rules
On Tue, Jun 22, 2021 at 5:22 PM Mark Michelson 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
> >>> ---
> >>> 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
> >>
> >> 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
> #include
>
> 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();
> ds_put_format_valist(, match_fmt, ap);
>
> struct va_list aq;
> va_copy(aq, ap);
> ds_clear();
> ds_put_format_valist(, 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
Re: [ovs-dev] [PATCH ovn] northd: avoid memory reallocation while building ACL and QoS rules
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
---
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
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.
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
#include
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();
ds_put_format_valist(, match_fmt, ap);
struct va_list aq;
va_copy(aq, ap);
ds_clear();
ds_put_format_valist(, 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
Re: [ovs-dev] [PATCH ovn] northd: avoid memory reallocation while building ACL and QoS rules
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 > > --- > > 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 > > 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 > > CC-ing Ilya too, maybe he has some more suggestions, maybe there's a > way > to better use the OVS dynamic strings. > > Regards, > Dumitru > > [0] > https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384043.html > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: avoid memory reallocation while building ACL and QoS rules
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 > --- > 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 However, I wonder how many such optimizations we can implement without affecting maintainability. Mark suggested an approach [0]. CC-ing Ilya too, maybe he has some more suggestions, maybe there's a way to better use the OVS dynamic strings. Regards, Dumitru [0] https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384043.html ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: avoid memory reallocation while building ACL and QoS rules
Bleep bloop. Greetings Dan Williams, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #338 FILE: northd/ovn-northd.c:5922: dhcp6_actions, >nbs->ports[i]->dhcpv6_options->header_); Lines checked: 445, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
