From patchwork Tue Aug 20 10:30:55 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Damijan Skvarc X-Patchwork-Id: 1150010 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b="Dl+1X+Wi"; dkim-atps=neutral Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 46CRss49d8z9sDQ for ; Tue, 20 Aug 2019 20:31:28 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id F1EA1D88; Tue, 20 Aug 2019 10:31:24 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id EE3DDD4A for ; Tue, 20 Aug 2019 10:31:22 +0000 (UTC) X-Greylist: whitelisted by SQLgrey-1.7.6 Received: from mail-ed1-f67.google.com (mail-ed1-f67.google.com [209.85.208.67]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 4D63689B for ; Tue, 20 Aug 2019 10:31:22 +0000 (UTC) Received: by mail-ed1-f67.google.com with SMTP id x19so5707875eda.12 for ; Tue, 20 Aug 2019 03:31:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=KNG5W65hy4hT5CI8PNZOZe02D58CjD2Pg+eZLN1avSg=; b=Dl+1X+Wi7DzXS0EPCgVQjn9dYlGA3qfbcPESMXbklZqtPzEA7AwIRS7fnUIA1tVMg2 j0CY81T4C7CqbNoYEy5Kefz5bAxiMUXUumyM+U6Ory9yioOuXdJ+0toyH9Z1Lv2raS1o K9+B2ISV9DojkCqEqDyZjRYFLhJ7A0JTYE6jSAwSHpmOlQ9bdloGye5etwYzxNAhdS5m c+CY7dauQ6/fDG2FtWS4doL2y5E+SIwf01TU1J27esgavtW9+u9tkpFgT36AlM90K1c/ tbx5g2A5Kh2kKBeatvwd2PuWEzFeTkR8dOqP9pOBwd7Rdz0SYnr+OEYGHvadtScc+j3l Q5lQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=KNG5W65hy4hT5CI8PNZOZe02D58CjD2Pg+eZLN1avSg=; b=qyZMETRN2GShWLDTf6IeaxID3596aBkGyh850afVOWzXY+sy9NjJ9e7jX+zxcFHc7u xtbIhJdW6kNz+READbH8fGY3Wl1EQeY5lH1DpcRmM7lPt2SVYmjOH1qchV/7Aeu526MZ A8n/cBYlN9e5rhwUPwFUmaJSRP0tO6IXTWDJtvzYd+JYTBOsQQ5MScEGWpZnmlmpnKuI UaWSPfuBiWzAdmPKmDa+0hXQu0udObqeAHSIf3brIQXxoDMD/E5e2YENquq5egzgI+vF mnqGz8WsRSV/PjQNPwvFieMWGh2Ndc9WsjjHUWbu3cOtc+5PcpzjRf1H+Lj4nHW58vQ/ j/wg== X-Gm-Message-State: APjAAAWvJ1kqFFKA0b2Lm5tkC+RkDXiOQ6uByH8mF1tdfJ69e4KTdQqr BMYqe5gg+bnFSVXboRHD4qLFpsz7YGs= X-Google-Smtp-Source: APXvYqxc4Bf4AGPFvPN8Meyx66m6P1d7/DebL/x7FplFSrdR49YDC+XqQ/yL9nYvUVyawtEBPm0CTA== X-Received: by 2002:a17:906:7d3:: with SMTP id m19mr25494288ejc.70.1566297080461; Tue, 20 Aug 2019 03:31:20 -0700 (PDT) Received: from damijan-PC.i-tech.local (mail.i-tech.si. [89.212.78.105]) by smtp.gmail.com with ESMTPSA id e24sm2536717ejb.53.2019.08.20.03.31.18 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 20 Aug 2019 03:31:19 -0700 (PDT) From: Damijan Skvarc To: dev@openvswitch.org, Ilya Maximets Date: Tue, 20 Aug 2019 12:30:55 +0200 Message-Id: <1566297055-13248-1-git-send-email-damjan.skvarc@gmail.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <90243416-36cd-897f-b878-652c0ca5ee99@samsung.com> References: <90243416-36cd-897f-b878-652c0ca5ee99@samsung.com> X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: [ovs-dev] [PATCH 1/1] [PATCH v3 ovn] logical-fields: fix memory leak while initializing ovnfield_by_name X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org >This patch named "fix memory leak ...", but I don't see any leaks fixed >in the code. It seems that you've sent diff between v1 and v2 instead of >the patch itself. If so, you need to squash this into your v1 patch and >send as v3. > >Best regards, Ilya Maximets. Yes, Ilya, you are right. I didn't realize patch file must contain cumulative changes (and not partial one) in case they are spread through several commits. In our case the memory leak occurs because ovnfield_by_name hash is initialized twice without destroying previously allocated memory. I know the problem can be solved in different ways (ovsthread_once, OVS_CONSTRUCTOR, calling ovn_destroy_ovnfields() before reinitializing it again,...). However my intention in this patch is just to solve memory leak and not changing anything in the behaviour. I am also aware that memory leak is not important since it happens only once during lifecycle of ovn-controller. Thus the one, who will get the most from this patch is the observer of valgrind reports while running test suite. Namely, while running test suite ovn-controller is restarted so many times and this memory leak is reported so many times that it obscure much more important issues. If anything else needs to be done regarding initialization of ovnfield_by_name hash I would reather think about getting rid of it, since it maps into ovn_fields array which is currently contituted from one entry only. I believe searching for field name in this single entry table directly would be even quicker then using hash. But since I don't know about the future/plans of ovn_fields array I am not able to do this change. Signed-off-by: Damijan Skvarc --- lib/logical-fields.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 8fb591c..13e5b83 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -57,6 +57,22 @@ add_ct_bit(const char *name, int index, struct shash *symtab) free(expansion); } +static void +ovn_init_ovnfields(void) +{ + static bool initialized = false; + + if (!initialized) { + shash_init(&ovnfield_by_name); + for (int i = 0; i < OVN_FIELD_N_IDS; i++) { + const struct ovn_field *of = &ovn_fields[i]; + ovs_assert(of->id == i); /* Fields must be in the enum order. */ + shash_add_once(&ovnfield_by_name, of->name, of); + } + initialized = true; + } +} + void ovn_init_symtab(struct shash *symtab) { @@ -220,12 +236,8 @@ ovn_init_symtab(struct shash *symtab) expr_symtab_add_field(symtab, "sctp.src", MFF_SCTP_SRC, "sctp", false); expr_symtab_add_field(symtab, "sctp.dst", MFF_SCTP_DST, "sctp", false); - shash_init(&ovnfield_by_name); - for (int i = 0; i < OVN_FIELD_N_IDS; i++) { - const struct ovn_field *of = &ovn_fields[i]; - ovs_assert(of->id == i); /* Fields must be in the enum order. */ - shash_add_once(&ovnfield_by_name, of->name, of); - } + ovn_init_ovnfields(); + expr_symtab_add_ovn_field(symtab, "icmp4.frag_mtu", OVN_ICMP4_FRAG_MTU); }