From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933045AbcFOTxA (ORCPT ); Wed, 15 Jun 2016 15:53:00 -0400 Received: from ja.ssi.bg ([178.16.129.10]:36110 "EHLO ja.ssi.bg" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753161AbcFOTw6 (ORCPT ); Wed, 15 Jun 2016 15:52:58 -0400 Date: Wed, 15 Jun 2016 22:52:22 +0300 (EEST) From: Julian Anastasov To: Quentin Armitage cc: Wensong Zhang , Simon Horman , Pablo Neira Ayuso , Patrick McHardy , Jozsef Kadlecsik , "David S. Miller" , netdev@vger.kernel.org, lvs-devel@vger.kernel.org, netfilter-devel@vger.kernel.org, coreteam@netfilter.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 0/5] ipvs: fix backup sync daemon with IPv6, and minor updates In-Reply-To: <1465978975.2737.264.camel@samson1.armitage.org.uk> Message-ID: References: <1465914262-30112-1-git-send-email-quentin@armitage.org.uk> <1465978975.2737.264.camel@samson1.armitage.org.uk> User-Agent: Alpine 2.11 (LFD 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On Wed, 15 Jun 2016, Quentin Armitage wrote: > I am updating the patches in line with your comments, but I'm not sure about > a couple of points. > > Patch 4: > > You state that before bind(), such changes should be safe. However, from the > function make_send_sock(), when the functions set_mcast_if(), > set_mcast_loop(), set_mcast_ttl() and set_mcast_pmtudisc() are called before > connect(), they all lock the socket before modifying it. Patch 4 was > intended to make the setting of REUSE consistent. > > If the locking is not necessary, would it be better to remove the locks from > the set_mcast_...() functions referred to above. This is a slow path, so it does not matter much. There is no concurrent access to the socket, the only risk is some call into the stack that checks with lockdep for the missing lock. Such example but for another lock we already hold is ASSERT_RTNL in ip_mc_join_group. But for simple sk vars lock is not needed. You can safely remove locks before connect/bind if only sk fields are accessed directly. We can keep it only in join_mcast_group*(), especially because they are called after bind(). > Re patch 1 setting 'sock->sk->sk_bound_dev_if = ifindex;', I presume the > locking should be consistent with what is done in the other functions. It is a simple var, so it can work without lock. > Your comments on the above would be really helpful. > > Patch 5: > > You state 'The indentation of existing pr_info in both cases should not be > changed". I'm not clear exactly what that means. Does it mean that the > spaces at the beginning of the pr_info() strings which report group, port > and ttl should be removed? No, here is example from your patch: pr_info("sync thread started: state = MASTER, mcast_ifn = %s, " - "syncid = %d, id = %d\n", - ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id); + "syncid = %d, id = %d, maxlen = %d\n", + ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, + tinfo->id, ipvs->mcfg.sync_maxlen); "syncid = " was at the same column as "sync thread started", you added another tab, may be to align with the args in new pr_info. The result is: pr_info("sync thread started: state = MASTER, mcast_ifn = %s, " "syncid = %d, id = %d, maxlen = %d\n", ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id, ipvs->mcfg.sync_maxlen); <--- 2 TABs ---> But it should be: pr_info("sync thread started: state = MASTER, mcast_ifn = %s, " "syncid = %d, id = %d, maxlen = %d\n", ipvs->mcfg.mcast_ifn, ipvs->mcfg.syncid, tinfo->id, ipvs->mcfg.sync_maxlen); < 1 TAB> Also, the new pr_info calls exceed 80 columns. May be you can reduce the many spaces. Regards -- Julian Anastasov