[{"id":1760473,"web_url":"http://patchwork.ozlabs.org/comment/1760473/","msgid":"","list_archive_url":null,"date":"2017-08-30T20:31:00","subject":"Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of conditional\n\tmonitor_cond requests.","submitter":{"id":67699,"url":"http://patchwork.ozlabs.org/api/people/67699/","name":"Andy Zhou","email":"azhou@ovn.org"},"content":"On Wed, Aug 30, 2017 at 9:33 AM, Ben Pfaff wrote:\n> The current implementation of ovsdb-server caches only non-conditional\n> monitors, that is, monitors for every table row, not those that monitor\n> only rows that match some condition. To figure out which monitors are\n> conditional, the code track the number of tables that have conditions that\n> are uniformly true (cond->n_true_cnd) and compares that against the number\n> of tables in the condition (shash_count(&cond->tables)). If they are the\n> same, then every table has (effectively) no condition, and so\n> cond->conditional is set to false.\n>\n> However, the implementation was buggy. The function that adds a new\n> table condition, ovsdb_monitor_table_condition_create(), only updated\n> cond->conditional if the table condition being added was true. This is\n> wrong; only adding a non-true condition can actually change\n> cond->conditional. This commit fixes the problem by always recalculating\n> cond->conditional.\n>\n> The most visible side effect of cond->conditional being true when it\n> should be false, as caused by this bug, was that conditional monitors were\n> being mixed with unconditional monitors for the purpose of caching. This\n> meant that, if a client requested a conditional monitor that was the\n> same as an unconditional one, except for the condition, then the client\n> would receive the cached data previously sent for the unconditional one.\n> This commit fixes the problem.\n>\n> Signed-off-by: Ben Pfaff \n\nI think the first paragraph of the commit message can be useful as comments\nin the code as well.\n\nAcked-by: Andy Zhou ","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xjHGs4s3Pz9sNw\n\tfor ;\n\tThu, 31 Aug 2017 06:31:49 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 39BD2BF3;\n\tWed, 30 Aug 2017 20:31:46 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id 41F62989\n\tfor ; Wed, 30 Aug 2017 20:31:45 +0000 (UTC)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id C031EF6\n\tfor ; Wed, 30 Aug 2017 20:31:44 +0000 (UTC)","from mail-pg0-f53.google.com (mail-pg0-f53.google.com\n\t[74.125.83.53]) (Authenticated sender: azhou@ovn.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 0823AFB87D\n\tfor ; Wed, 30 Aug 2017 22:31:42 +0200 (CEST)","by mail-pg0-f53.google.com with SMTP id t3so22995170pgt.0\n\tfor ; Wed, 30 Aug 2017 13:31:42 -0700 (PDT)","by 10.100.163.195 with HTTP; Wed, 30 Aug 2017 13:31:00 -0700 (PDT)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"74.125.83.53","X-Gm-Message-State":"AHYfb5i+5zgD5l+upIOkAydC35YSMHJaYsYIxoUgs4eSaRCTYuCCbvH9\n\tmltsAoKTYeeQYWFbANLC/Nx/Je/cIw==","X-Received":"by 10.98.77.130 with SMTP id a124mr279779pfb.107.1504125101272; \n\tWed, 30 Aug 2017 13:31:41 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170830163314.1411-1-blp@ovn.org>","References":"<20170830163314.1411-1-blp@ovn.org>","From":"Andy Zhou ","Date":"Wed, 30 Aug 2017 13:31:00 -0700","X-Gmail-Original-Message-ID":"","Message-ID":"","To":"Ben Pfaff ","X-Spam-Status":"No, score=-0.2 required=5.0 tests=RCVD_IN_DNSWL_LOW,\n\tRCVD_IN_SORBS_SPAM autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"\"\" ","Subject":"Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of conditional\n\tmonitor_cond requests.","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1760680,"web_url":"http://patchwork.ozlabs.org/comment/1760680/","msgid":"","list_archive_url":null,"date":"2017-08-31T06:48:14","subject":"Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of\n\tconditional\tmonitor_cond requests.","submitter":{"id":2836,"url":"http://patchwork.ozlabs.org/api/people/2836/","name":"Liran Schour","email":"lirans@il.ibm.com"},"content":"ovs-dev-bounces@openvswitch.org wrote on 30/08/2017 07:33:13 PM:\n> The current implementation of ovsdb-server caches only non-conditional\n> monitors, that is, monitors for every table row, not those that monitor\n> only rows that match some condition. To figure out which monitors are\n> conditional, the code track the number of tables that have conditions \nthat\n> are uniformly true (cond->n_true_cnd) and compares that against the \nnumber\n> of tables in the condition (shash_count(&cond->tables)). If they are \nthe\n> same, then every table has (effectively) no condition, and so\n> cond->conditional is set to false.\n> \n> However, the implementation was buggy. The function that adds a new\n> table condition, ovsdb_monitor_table_condition_create(), only updated\n> cond->conditional if the table condition being added was true. This is\n> wrong; only adding a non-true condition can actually change\n> cond->conditional. This commit fixes the problem by always \nrecalculating\n> cond->conditional.\n> \n> The most visible side effect of cond->conditional being true when it\n> should be false, as caused by this bug, was that conditional monitors \nwere\n> being mixed with unconditional monitors for the purpose of caching. This\n> meant that, if a client requested a conditional monitor that was the\n> same as an unconditional one, except for the condition, then the client\n> would receive the cached data previously sent for the unconditional one.\n> This commit fixes the problem.\n> \n> Signed-off-by: Ben Pfaff \n> ---\n> ovsdb/monitor.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c\n> index b98100703091..7a5c2f905560 100644\n> --- a/ovsdb/monitor.c\n> +++ b/ovsdb/monitor.c\n> @@ -651,8 +651,8 @@ ovsdb_monitor_table_condition_create(\n> ovsdb_condition_clone(&mtc->new_condition, &mtc->old_condition);\n> if (ovsdb_condition_is_true(&mtc->old_condition)) {\n> condition->n_true_cnd++;\n> - ovsdb_monitor_session_condition_set_mode(condition);\n> }\n> + ovsdb_monitor_session_condition_set_mode(condition);\n> \n> return NULL;\n> }\n> -- \n> 2.10.2\n> \n\nYes. It is a bug. Thanks.\n\nAcked-by: Liran Schour ","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xjXyM1Y9tz9s7F\n\tfor ;\n\tThu, 31 Aug 2017 16:48:26 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 1CD62723;\n\tThu, 31 Aug 2017 06:48:22 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id D67964A5\n\tfor ; Thu, 31 Aug 2017 06:48:20 +0000 (UTC)","from mx0a-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com\n\t[148.163.158.5])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id BE40479\n\tfor ; Thu, 31 Aug 2017 06:48:19 +0000 (UTC)","from pps.filterd (m0098420.ppops.net [127.0.0.1])\n\tby mx0b-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv7V6lTSd133926\n\tfor ; Thu, 31 Aug 2017 02:48:18 -0400","from smtp.notes.na.collabserv.com (smtp.notes.na.collabserv.com\n\t[192.155.248.81])\n\tby mx0b-001b2d01.pphosted.com with ESMTP id 2cpdd688as-1\n\t(version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256\n\tverify=NOT)\n\tfor ; Thu, 31 Aug 2017 02:48:18 -0400","from localhost\n\tby smtp.notes.na.collabserv.com with smtp.notes.na.collabserv.com\n\tESMTP for from ;\n\tThu, 31 Aug 2017 06:48:18 -0000","from us1a3-smtp03.a3.dal06.isc4sb.com (10.106.154.98)\n\tby smtp.notes.na.collabserv.com (10.106.227.88) with\n\tsmtp.notes.na.collabserv.com ESMTP; Thu, 31 Aug 2017 06:48:15 -0000","from us1a3-mail116.a3.dal06.isc4sb.com ([10.146.45.125])\n\tby us1a3-smtp03.a3.dal06.isc4sb.com\n\twith ESMTP id 2017083106481511-135592 ;\n\tThu, 31 Aug 2017 06:48:15 +0000 "],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","In-Reply-To":"<20170830163314.1411-1-blp@ovn.org>","To":"Ben Pfaff ","From":"\"Liran Schour\" ","Date":"Thu, 31 Aug 2017 09:48:14 +0300","References":"<20170830163314.1411-1-blp@ovn.org>","MIME-Version":"1.0","X-KeepSent":"CAA62AF2:782CC25F-C225818D:00251798;\n type=4; name=$KeepSent","X-Mailer":"IBM Notes Release 9.0.1FP7 August 18, 2016","X-LLNOutbound":"False","X-Disclaimed":"63991","X-TNEFEvaluated":"1","x-cbid":"17083106-7093-0000-0000-0000032F2553","X-IBM-SpamModules-Scores":"BY=0; FL=0; FP=0; FZ=0; HX=0; KW=0; PH=0; SC=0.49; \n\tST=0; TS=0; UL=0; ISC=; MB=0.138560","X-IBM-SpamModules-Versions":"BY=3.00007640; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000226; SDB=6.00910085; UDB=6.00456503;\n\tIPR=6.00690366; \n\tBA=6.00005562; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016938;\n\tXFM=3.00000015; UTC=2017-08-31 06:48:17","X-IBM-AV-DETECTION":"SAVI=unsuspicious REMOTE=unsuspicious XFE=unused","X-IBM-AV-VERSION":"SAVI=2017-08-31 00:45:38 - 6.00007258","x-cbparentid":"17083106-7094-0000-0000-00000FA56895","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-08-31_01:, , signatures=0","X-Proofpoint-Spam-Reason":"safe","X-Spam-Status":"No, score=-0.7 required=5.0 tests=HTML_MESSAGE,\n\tRCVD_IN_DNSWL_LOW autolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","X-Content-Filtered-By":"Mailman/MimeDel 2.1.12","Cc":"dev@openvswitch.org","Subject":"Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of\n\tconditional\tmonitor_cond requests.","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}},{"id":1761091,"web_url":"http://patchwork.ozlabs.org/comment/1761091/","msgid":"<20170831145602.GJ6175@ovn.org>","list_archive_url":null,"date":"2017-08-31T14:56:02","subject":"Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of conditional\n\tmonitor_cond requests.","submitter":{"id":67603,"url":"http://patchwork.ozlabs.org/api/people/67603/","name":"Ben Pfaff","email":"blp@ovn.org"},"content":"On Thu, Aug 31, 2017 at 09:48:14AM +0300, Liran Schour wrote:\n> ovs-dev-bounces@openvswitch.org wrote on 30/08/2017 07:33:13 PM:\n> > The current implementation of ovsdb-server caches only non-conditional\n> > monitors, that is, monitors for every table row, not those that monitor\n> > only rows that match some condition. To figure out which monitors are\n> > conditional, the code track the number of tables that have conditions \n> that\n> > are uniformly true (cond->n_true_cnd) and compares that against the \n> number\n> > of tables in the condition (shash_count(&cond->tables)). If they are \n> the\n> > same, then every table has (effectively) no condition, and so\n> > cond->conditional is set to false.\n> > \n> > However, the implementation was buggy. The function that adds a new\n> > table condition, ovsdb_monitor_table_condition_create(), only updated\n> > cond->conditional if the table condition being added was true. This is\n> > wrong; only adding a non-true condition can actually change\n> > cond->conditional. This commit fixes the problem by always \n> recalculating\n> > cond->conditional.\n> > \n> > The most visible side effect of cond->conditional being true when it\n> > should be false, as caused by this bug, was that conditional monitors \n> were\n> > being mixed with unconditional monitors for the purpose of caching. This\n> > meant that, if a client requested a conditional monitor that was the\n> > same as an unconditional one, except for the condition, then the client\n> > would receive the cached data previously sent for the unconditional one.\n> > This commit fixes the problem.\n> > \n> > Signed-off-by: Ben Pfaff \n> > ---\n> > ovsdb/monitor.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c\n> > index b98100703091..7a5c2f905560 100644\n> > --- a/ovsdb/monitor.c\n> > +++ b/ovsdb/monitor.c\n> > @@ -651,8 +651,8 @@ ovsdb_monitor_table_condition_create(\n> > ovsdb_condition_clone(&mtc->new_condition, &mtc->old_condition);\n> > if (ovsdb_condition_is_true(&mtc->old_condition)) {\n> > condition->n_true_cnd++;\n> > - ovsdb_monitor_session_condition_set_mode(condition);\n> > }\n> > + ovsdb_monitor_session_condition_set_mode(condition);\n> > \n> > return NULL;\n> > }\n> > -- \n> > 2.10.2\n> > \n> \n> Yes. It is a bug. Thanks.\n> \n> Acked-by: Liran Schour \n> \n\nThanks Andy and Liran, I applied this to master and as far back as\nbranch-2.6.","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","dev@openvswitch.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","ovs-dev@mail.linuxfoundation.org"],"Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=openvswitch.org\n\t(client-ip=140.211.169.12; helo=mail.linuxfoundation.org;\n\tenvelope-from=ovs-dev-bounces@openvswitch.org;\n\treceiver=)","Received":["from mail.linuxfoundation.org (mail.linuxfoundation.org\n\t[140.211.169.12])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xjlnG0Wgqz9s75\n\tfor ;\n\tFri, 1 Sep 2017 00:56:16 +1000 (AEST)","from mail.linux-foundation.org (localhost [127.0.0.1])\n\tby mail.linuxfoundation.org (Postfix) with ESMTP id 4F303AC8;\n\tThu, 31 Aug 2017 14:56:15 +0000 (UTC)","from smtp1.linuxfoundation.org (smtp1.linux-foundation.org\n\t[172.17.192.35])\n\tby mail.linuxfoundation.org (Postfix) with ESMTPS id B23414A6\n\tfor ; Thu, 31 Aug 2017 14:56:13 +0000 (UTC)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby smtp1.linuxfoundation.org (Postfix) with ESMTPS id 22F13428\n\tfor ; Thu, 31 Aug 2017 14:56:13 +0000 (UTC)","from ovn.org (173-228-112-34.dsl.dynamic.fusionbroadband.com\n\t[173.228.112.34]) (Authenticated sender: blp@ovn.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A9E6B1720C2;\n\tThu, 31 Aug 2017 16:56:07 +0200 (CEST)"],"X-Greylist":"domain auto-whitelisted by SQLgrey-1.7.6","X-Originating-IP":"173.228.112.34","Date":"Thu, 31 Aug 2017 07:56:02 -0700","From":"Ben Pfaff ","To":"Liran Schour , Andy Zhou ","Message-ID":"<20170831145602.GJ6175@ovn.org>","References":"<20170830163314.1411-1-blp@ovn.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-Spam-Status":"No, score=-0.7 required=5.0 tests=RCVD_IN_DNSWL_LOW\n\tautolearn=disabled version=3.3.1","X-Spam-Checker-Version":"SpamAssassin 3.3.1 (2010-03-16) on\n\tsmtp1.linux-foundation.org","Cc":"dev@openvswitch.org","Subject":"Re: [ovs-dev] [PATCH 1/2] monitor: Fix bad caching of conditional\n\tmonitor_cond requests.","X-BeenThere":"ovs-dev@openvswitch.org","X-Mailman-Version":"2.1.12","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Sender":"ovs-dev-bounces@openvswitch.org","Errors-To":"ovs-dev-bounces@openvswitch.org"}}]