On 8/30/23 21:28, Paolo Valerio wrote:
> Ilya Maximets <[email protected]> writes:
>
>> On 8/23/23 14:53, Paolo Valerio wrote:
>>> From: hepeng <[email protected]>
>>>
>>> The patch avoids the extra allocation for nat_conn.
>>> Currently, when doing NAT, the userspace conntrack will use an extra
>>> conn for the two directions in a flow. However, each conn has actually
>>> the two keys for both orig and rev directions. This patch introduces a
>>> key_node[CT_DIRS] member as per Aaron's suggestion in the conn which
>>> consists of a key, direction, and a cmap_node for hash lookup so
>>> addressing the feedback received by the original patch [0].
>>>
>>> [0]
>>> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
>>>
>>> Signed-off-by: Peng He <[email protected]>
>>> Co-authored-by: Paolo Valerio <[email protected]>
>>> Signed-off-by: Paolo Valerio <[email protected]>
>>> ---
>>> v2:
>>> - use enum value instead of bool (Aaron).
>>> - s/conn_for_expectation/conn_for_exp/ in process_ftp_ctl_v6()
>>> to avoid long line.
>>> - removed CT_CONN_TYPE_* reference in two comments.
>>> ---
>>> lib/conntrack-private.h | 19 +--
>>> lib/conntrack-tp.c | 6 +
>>> lib/conntrack.c | 350
>>> +++++++++++++++++++----------------------------
>>> 3 files changed, 155 insertions(+), 220 deletions(-)
<snip>
>>> @@ -585,34 +528,34 @@ conn_key_lookup(struct conntrack *ct, const struct
>>> conn_key *key,
>>> uint32_t hash, long long now, struct conn **conn_out,
>>> bool *reply)
>>> {
>>> - struct conn *conn;
>>> + struct conn_key_node *keyn;
>>> + struct conn *conn = NULL;
>>> bool found = false;
>>>
>>> - CMAP_FOR_EACH_WITH_HASH (conn, cm_node, hash, &ct->conns) {
>>> + CMAP_FOR_EACH_WITH_HASH (keyn, cm_node, hash, &ct->conns) {
>>> + conn = CONTAINER_OF(keyn, struct conn, key_node[keyn->dir]);
>>
>> Hmm, that's a neat trick, but I'm not sure we're allowed to do that.
>>
>> CONTAINER_OF involves offsetof, and offsetof according to documentation
>> supposed to expand into 'integer constant expression'. It is allowed
>> to have nested types and array indexes, but I was unable to find any
>> reference to using a variable as an index. And there is no way for an
>> expression that contains a variable to be expanded into integer constant
>> expression. So, I'm wondering if this line constitutes an undefined
>> behavior?
>>
>> Me might have to put it into an if expression in order to have a
>> compile-time constant offsetof. i.e.
>>
>> if (keyn->dir == CT_DIR_FWD) {
>> conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_FWD]);
>> } else {
>> conn = CONTAINER_OF(keyn, struct conn, key_node[CT_DIR_REV]);
>> }
>>
>> Same for all other occurances.
>>
>> Thoughts?
>>
>
> I think that's a good catch. Although this would probably not be an
> issue in most implementations (I tested multiple versions of both gcc
> and clang), the "integer constant expression" makes any assumption weak
> and probably unreliable.
>
> We'd better use what you suggest for this.
FWIW, found this comp.lang.c discussion:
https://groups.google.com/g/comp.lang.c/c/kwXeaY2HtB0
People seem to generally agree that it's an undefined behavior.
And there seems to be at least one compiler (TI) that doesn't
like that code. Though that compiler seems to be not compliant
to the standard in many other ways. :D
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
