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;
Date:   Wed, 9 Nov 2022 18:28:22 -0800
From:   Peilin Ye <yepeilin.cs@gmail.com>
To:     Leon Romanovsky <leon@kernel.org>
Cc:     Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
        David Ahern <dsahern@kernel.org>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Peilin Ye <peilin.ye@bytedance.com>,
        Cong Wang <cong.wang@bytedance.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5] net/sock: Introduce trace_sk_data_ready()
Message-ID: <20221110022822.GA2463@bytedance>
References: <20221012232121.27374-1-yepeilin.cs@gmail.com>
 <20221014000058.30060-1-yepeilin.cs@gmail.com>
 <Y0kDKpuJHPC36kal@unreal>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y0kDKpuJHPC36kal@unreal>
Precedence: bulk

On Fri, Oct 14, 2022 at 09:35:22AM +0300, Leon Romanovsky wrote:
> Please don't reply-to new patches and always send them as new threads
> with links to previous versions in changelog.

Sure, I can do that.  However, for example: 

  - I got a build error.
  - I found on LKML some v1 patch developed by someone else that
    introduced a similar error, by searching the error message.
  - I wanted to know how v2 fixed that error in v1, but since v2 didn't
    in-reply-to v1, it took me some extra seconds to find v2.

Therefore, I think sometimes it's useful to keep all versions in one
thread, especially when the set only contains one patch?

> > diff --git a/drivers/infiniband/hw/erdma/erdma_cm.c b/drivers/infiniband/hw/erdma/erdma_cm.c
> > index f13f16479eca..084da6698080 100644
> > --- a/drivers/infiniband/hw/erdma/erdma_cm.c
> > +++ b/drivers/infiniband/hw/erdma/erdma_cm.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/types.h>
> >  #include <linux/workqueue.h>
> >  #include <net/addrconf.h>
> > +#include <trace/events/sock.h>
> >  
> >  #include <rdma/ib_user_verbs.h>
> >  #include <rdma/ib_verbs.h>
> > @@ -933,6 +934,8 @@ static void erdma_cm_llp_data_ready(struct sock *sk)
> >  {
> >  	struct erdma_cep *cep;
> >  
> > +	trace_sk_data_ready(sk);
> > +
> >  	read_lock(&sk->sk_callback_lock);
> 
> I see this pattern in all places and don't know if it is correct or not,
> but you are calling to trace_sk_data_ready() at the beginning of
> function and do it without taking sk_callback_lock.

Thanks for bringing this up, but I'm not sure it's an issue.  We already
do similar thing, for example, in net/core/neighbour.c:

static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
			  u8 new, u32 flags, u32 nlmsg_pid,
			  struct netlink_ext_ack *extack)
{
	bool gc_update = false, managed_update = false;
	int update_isrouter = 0;
	struct net_device *dev;
	int err, notify = 0;
	u8 old;

	trace_neigh_update(neigh, lladdr, new, flags, nlmsg_pid);

	write_lock_bh(&neigh->lock);

Thanks,
Peilin Ye