Re: Fwd: uprobes: register/unregister probes.

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

> 
> On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > +#define UPROBES_HASH_SZ        13
> > +/* serialize (un)register */
> > +static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
> > +#define uprobes_hash(v)        (&uprobes_mutex[((unsigned long)(v)) %\
> > +                                               UPROBES_HASH_SZ])
> 
> Was there any reason to for using this hasing scheme, say over hash.h?

There is no specific reason for choosing this hashing scheme over the
current. I just say ext4_aio_mutex in fs/ext4/ext4.h and did something
similar.

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

> 
> On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > +#define UPROBES_HASH_SZ        13
> > +/* serialize (un)register */
> > +static struct mutex uprobes_mutex[UPROBES_HASH_SZ];
> > +#define uprobes_hash(v)        (&uprobes_mutex[((unsigned long)(v)) %\
> > +                                               UPROBES_HASH_SZ])
> 
> Was there any reason to for using this hasing scheme, say over hash.h?

There is no specific reason for choosing this hashing scheme over the
current. I just say ext4_aio_mutex in fs/ext4/ext4.h and did something
similar.

-- 
Thanks and Regards
Srikar

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

> On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > +int register_uprobe(struct inode *inode, loff_t offset,
> > +                               struct uprobe_consumer *consumer)
> > +{
> > +       struct uprobe *uprobe;
> > +       int ret = -EINVAL;
> > +
> > +       if (!consumer || consumer->next)
> > +               return ret;
> > +
> > +       inode = igrab(inode);
> 
> So why are you dealing with !consumer but not with !inode? and why
> does
> it make sense to allow !consumer at all?
> 


I am not sure if I got your comment correctly.

I do check for inode just after the igrab.

I am actually not dealing with !consumer.
If the consumer is NULL, then we dont have any handler to run so why
would we want to register such a probe?

Also if consumer->next is Non-NULL, that means that this consumer was
already used.  Reusing the consumer, can result in consumers list getting
broken into two.

If you meant something else please clarify.

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

> On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > +int register_uprobe(struct inode *inode, loff_t offset,
> > +                               struct uprobe_consumer *consumer)
> > +{
> > +       struct uprobe *uprobe;
> > +       int ret = -EINVAL;
> > +
> > +       if (!consumer || consumer->next)
> > +               return ret;
> > +
> > +       inode = igrab(inode);
> 
> So why are you dealing with !consumer but not with !inode? and why
> does
> it make sense to allow !consumer at all?
> 


I am not sure if I got your comment correctly.

I do check for inode just after the igrab.

I am actually not dealing with !consumer.
If the consumer is NULL, then we dont have any handler to run so why
would we want to register such a probe?

Also if consumer->next is Non-NULL, that means that this consumer was
already used.  Reusing the consumer, can result in consumers list getting
broken into two.

If you meant something else please clarify.

-- 
Thanks and Regards
Srikar

Re: Fwd: uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-11-24 at 12:33 +0530, Srikar Dronamraju wrote:
> > On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > > +int register_uprobe(struct inode *inode, loff_t offset,
> > > +                               struct uprobe_consumer *consumer)
> > > +{
> > > +       struct uprobe *uprobe;
> > > +       int ret = -EINVAL;
> > > +
> > > +       if (!consumer || consumer->next)
> > > +               return ret;
> > > +
> > > +       inode = igrab(inode);
> > 
> > So why are you dealing with !consumer but not with !inode? and why
> > does
> > it make sense to allow !consumer at all?
> > 
> 
> 
> I am not sure if I got your comment correctly.
> 
> I do check for inode just after the igrab.

No you don't, you check the return value of igrab(), but you crash hard
when someone calls register_uprobe(.inode=NULL).

> I am actually not dealing with !consumer.
> If the consumer is NULL, then we dont have any handler to run so why
> would we want to register such a probe?

Why allow someone calling register_uprobe(.consumer=NULL) to begin with?
That doesn't make any sense.

> Also if consumer->next is Non-NULL, that means that this consumer was
> already used.  Reusing the consumer, can result in consumers list getting
> broken into two.

Yeah, although at that point why be nice about it? Just but a WARN_ON()
in or so.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-11-24 at 12:33 +0530, Srikar Dronamraju wrote:
> > On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > > +int register_uprobe(struct inode *inode, loff_t offset,
> > > +                               struct uprobe_consumer *consumer)
> > > +{
> > > +       struct uprobe *uprobe;
> > > +       int ret = -EINVAL;
> > > +
> > > +       if (!consumer || consumer->next)
> > > +               return ret;
> > > +
> > > +       inode = igrab(inode);
> > 
> > So why are you dealing with !consumer but not with !inode? and why
> > does
> > it make sense to allow !consumer at all?
> > 
> 
> 
> I am not sure if I got your comment correctly.
> 
> I do check for inode just after the igrab.

No you don't, you check the return value of igrab(), but you crash hard
when someone calls register_uprobe(.inode=NULL).

> I am actually not dealing with !consumer.
> If the consumer is NULL, then we dont have any handler to run so why
> would we want to register such a probe?

Why allow someone calling register_uprobe(.consumer=NULL) to begin with?
That doesn't make any sense.

> Also if consumer->next is Non-NULL, that means that this consumer was
> already used.  Reusing the consumer, can result in consumers list getting
> broken into two.

Yeah, although at that point why be nice about it? Just but a WARN_ON()
in or so.

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-11-24 10:49:59]:

> On Thu, 2011-11-24 at 12:33 +0530, Srikar Dronamraju wrote:
> > > On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > > > +int register_uprobe(struct inode *inode, loff_t offset,
> > > > +                               struct uprobe_consumer *consumer)
> > > > +{
> > > > +       struct uprobe *uprobe;
> > > > +       int ret = -EINVAL;
> > > > +
> > > > +       if (!consumer || consumer->next)
> > > > +               return ret;
> > > > +
> > > > +       inode = igrab(inode);
> > > 
> > > So why are you dealing with !consumer but not with !inode? and why
> > > does
> > > it make sense to allow !consumer at all?
> > > 
> > 
> > 
> > I am not sure if I got your comment correctly.
> > 
> > I do check for inode just after the igrab.
> 
> No you don't, you check the return value of igrab(), but you crash hard
> when someone calls register_uprobe(.inode=NULL).
> 

Okay. will add a check for inode before we do the igrab.

> > I am actually not dealing with !consumer.
> > If the consumer is NULL, then we dont have any handler to run so why
> > would we want to register such a probe?
> 
> Why allow someone calling register_uprobe(.consumer=NULL) to begin with?
> That doesn't make any sense.
> 
> > Also if consumer->next is Non-NULL, that means that this consumer was
> > already used.  Reusing the consumer, can result in consumers list getting
> > broken into two.
> 
> Yeah, although at that point why be nice about it? Just but a WARN_ON()
> in or so.
> 

I thought you werent happy with prints and WARN_ON/BUG_ON unless it was
really really necessary. If we were to have a WARN_ON for a wrong
consumer passed, then we would need a warn_On for NULL inode too.

So I think we should leave this as is.

Unless I have commented, I agree to your comments sent in other threads
and will resolve them accordingly.

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

* Peter Zijlstra <peterz@infradead.org> [2011-11-24 10:49:59]:

> On Thu, 2011-11-24 at 12:33 +0530, Srikar Dronamraju wrote:
> > > On Fri, 2011-11-18 at 16:37 +0530, Srikar Dronamraju wrote:
> > > > +int register_uprobe(struct inode *inode, loff_t offset,
> > > > +                               struct uprobe_consumer *consumer)
> > > > +{
> > > > +       struct uprobe *uprobe;
> > > > +       int ret = -EINVAL;
> > > > +
> > > > +       if (!consumer || consumer->next)
> > > > +               return ret;
> > > > +
> > > > +       inode = igrab(inode);
> > > 
> > > So why are you dealing with !consumer but not with !inode? and why
> > > does
> > > it make sense to allow !consumer at all?
> > > 
> > 
> > 
> > I am not sure if I got your comment correctly.
> > 
> > I do check for inode just after the igrab.
> 
> No you don't, you check the return value of igrab(), but you crash hard
> when someone calls register_uprobe(.inode=NULL).
> 

Okay. will add a check for inode before we do the igrab.

> > I am actually not dealing with !consumer.
> > If the consumer is NULL, then we dont have any handler to run so why
> > would we want to register such a probe?
> 
> Why allow someone calling register_uprobe(.consumer=NULL) to begin with?
> That doesn't make any sense.
> 
> > Also if consumer->next is Non-NULL, that means that this consumer was
> > already used.  Reusing the consumer, can result in consumers list getting
> > broken into two.
> 
> Yeah, although at that point why be nice about it? Just but a WARN_ON()
> in or so.
> 

I thought you werent happy with prints and WARN_ON/BUG_ON unless it was
really really necessary. If we were to have a WARN_ON for a wrong
consumer passed, then we would need a warn_On for NULL inode too.

So I think we should leave this as is.

Unless I have commented, I agree to your comments sent in other threads
and will resolve them accordingly.

-- 
Thanks and Regards
Srikar

Re: Fwd: uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-11-24 at 20:21 +0530, Srikar Dronamraju wrote:
> > No you don't, you check the return value of igrab(), but you crash hard
> > when someone calls register_uprobe(.inode=NULL).
> > 
> 
> Okay. will add a check for inode before we do the igrab. 

No!!! its fcking pointless calling this function without a valid inode
argument, don't mess about and try and deal with it.

Same with the consumer thing, if you call it with a NULL consumer you're
an idiot, try memcpy(NULL, foo, size), does that return -EINVAL?

Also, what's the point of all this igrab() nonsense? We don't need extra
references on the inode, the caller of these functions had better made
sure the inode is stable and good to use, otherwise it could be freed
before we do igrab() and we'd still crash.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-11-24 at 20:21 +0530, Srikar Dronamraju wrote:
> > No you don't, you check the return value of igrab(), but you crash hard
> > when someone calls register_uprobe(.inode=NULL).
> > 
> 
> Okay. will add a check for inode before we do the igrab. 

No!!! its fcking pointless calling this function without a valid inode
argument, don't mess about and try and deal with it.

Same with the consumer thing, if you call it with a NULL consumer you're
an idiot, try memcpy(NULL, foo, size), does that return -EINVAL?

Also, what's the point of all this igrab() nonsense? We don't need extra
references on the inode, the caller of these functions had better made
sure the inode is stable and good to use, otherwise it could be freed
before we do igrab() and we'd still crash.

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

> > +
> > +       mutex_unlock(uprobes_hash(inode));
> > +       put_uprobe(uprobe);
> > +
> > +reg_out:
> > +       iput(inode);
> > +       return ret;
> > +}
> 
> So if this function returns an error the caller is responsible for
> cleaning up consumer, otherwise we take responsibility.

The caller is always responsible to cleanup the consumer. 
The only field we touch in the consumer is the next; thats because 
we use to link up the consumers.

-- 
Thanks and Regards
Srikar

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Srikar Dronamraju]

> > +
> > +       mutex_unlock(uprobes_hash(inode));
> > +       put_uprobe(uprobe);
> > +
> > +reg_out:
> > +       iput(inode);
> > +       return ret;
> > +}
> 
> So if this function returns an error the caller is responsible for
> cleaning up consumer, otherwise we take responsibility.

The caller is always responsible to cleanup the consumer. 
The only field we touch in the consumer is the next; thats because 
we use to link up the consumers.

-- 
Thanks and Regards
Srikar

Re: Fwd: uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-11-24 at 15:19 +0530, Srikar Dronamraju wrote:
>                           Subject: 
> Re: Fwd: uprobes:
> register/unregister probes.
> 
Why is this a fwd and breaking threading?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Fwd: uprobes: register/unregister probes.

[Peter Zijlstra]

On Thu, 2011-11-24 at 15:19 +0530, Srikar Dronamraju wrote:
>                           Subject: 
> Re: Fwd: uprobes:
> register/unregister probes.
> 
Why is this a fwd and breaking threading?