[ovs-dev] [PATCH ovn] northd: avoid memory reallocation while building ACL and QoS rules

[Mark Michelson]

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 <dcbw at 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 <dceara at 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.

> 
>>
>> 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