Re: [PATCH net-next v5] net/sock: Introduce trace_sk_data_ready()

[Peilin Ye]

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