Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4791cc31-db17-7720-4a86-f83e7bf0918d@hartkopp.net>
Date:   Mon, 12 Sep 2022 16:54:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH 1/2] can: bcm: registration process optimization in
 bcm_module_init()
Content-Language: en-US
To:     Marc Kleine-Budde <mkl@pengutronix.de>
Cc:     "Ziyang Xuan (William)" <william.xuanziyang@huawei.com>,
        edumazet@google.com, kuba@kernel.org, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <cover.1662606045.git.william.xuanziyang@huawei.com>
 <823cff0ebec33fa9389eeaf8b8ded3217c32cb38.1662606045.git.william.xuanziyang@huawei.com>
 <381dd961-f786-2400-0977-9639c3f7006e@hartkopp.net>
 <c480bdd7-e35e-fbf9-6767-801e04703780@hartkopp.net>
 <7b063d38-311c-76d6-4e31-02f9cccc9bcb@huawei.com>
 <053c7de3-c76c-82fd-2d44-2e7c1673ae98@hartkopp.net>
 <9228b20a-3baa-32ad-6059-5cf0ffdb97a3@huawei.com>
 <d392c1f4-7ad3-59a4-1358-2c216c498402@hartkopp.net>
 <20220912120020.dlxuryltw4sii635@pengutronix.de>
From:   Oliver Hartkopp <socketcan@hartkopp.net>
In-Reply-To: <20220912120020.dlxuryltw4sii635@pengutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 12.09.22 14:00, Marc Kleine-Budde wrote:
> On 09.09.2022 17:04:06, Oliver Hartkopp wrote:
>>
>>
>> On 09.09.22 05:58, Ziyang Xuan (William) wrote:
>>>>
>>>>
>>>> On 9/8/22 13:14, Ziyang Xuan (William) wrote:
>>>>>> Just another reference which make it clear that the reordering of function calls in your patch is likely not correct:
>>>>>>
>>>>>> https://elixir.bootlin.com/linux/v5.19.7/source/net/packet/af_packet.c#L4734
>>>>>>
>>>>>> static int __init packet_init(void)
>>>>>> {
>>>>>>            int rc;
>>>>>>
>>>>>>            rc = proto_register(&packet_proto, 0);
>>>>>>            if (rc)
>>>>>>                    goto out;
>>>>>>            rc = sock_register(&packet_family_ops);
>>>>>>            if (rc)
>>>>>>                    goto out_proto;
>>>>>>            rc = register_pernet_subsys(&packet_net_ops);
>>>>>>            if (rc)
>>>>>>                    goto out_sock;
>>>>>>            rc = register_netdevice_notifier(&packet_netdev_notifier);
>>>>>>            if (rc)
>>>>>>                    goto out_pernet;
>>>>>>
>>>>>>            return 0;
>>>>>>
>>>>>> out_pernet:
>>>>>>            unregister_pernet_subsys(&packet_net_ops);
>>>>>> out_sock:
>>>>>>            sock_unregister(PF_PACKET);
>>>>>> out_proto:
>>>>>>            proto_unregister(&packet_proto);
>>>>>> out:
>>>>>>            return rc;
>>>>>> }
>>>>>>
>>
>>> Yes，all these socket operations need time, most likely, register_netdevice_notifier() and register_pernet_subsys() had been done.
>>> But it maybe not for some reasons, for example, cpu# that runs {raw,bcm}_module_init() is stuck temporary,
>>> or pernet_ops_rwsem lock competition in register_netdevice_notifier() and register_pernet_subsys().
>>>
>>> If the condition which I pointed happens, I think my solution can solve.
>>>
>>
>> No, I don't think so.
>>
>> We need to maintain the exact order which is depicted in the af_packet.c
>> code from above as the notifier call references the sock pointer.
> 
> The notifier calls bcm_notifier() first, which will loop over the
> bcm_notifier_list. The list is empty if there are no sockets open, yet.
> So from my point of view this change looks fine.
> 
> IMHO it's better to make a series where all these notifiers are moved in
> front of the respective socket proto_register().

Notifiers and/or pernet_subsys ?

But yes, that would be better to have a clean consistent sequence in all 
these cases.

Would this affect af_packet.c then too?

Regards,
Oliver