Re: [PATCH 2.6.12-rc3-mm3] connector: add a fork connector

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

On Mon, 09 May 2005 13:38:44 +0200
Guillaume Thouvenin <guillaume.thouvenin@bull.net> wrote:

> On Mon, 2005-05-09 at 11:31 +0200, Alexander Nyberg wrote:
> > > +static inline void fork_connector(pid_t parent, pid_t child)
> > > +{
> > > +	if (cn_fork_enable) {
> > > +		struct cn_msg *msg;
> > > +		struct cn_fork_msg *forkmsg;
> > > +		__u8 buffer[CN_FORK_MSG_SIZE];	
> > > +
> > > +		msg = (struct cn_msg *)buffer;
> > > +			
> > > +		memcpy(&msg->id, &cb_fork_id, sizeof(msg->id));
> > > +		
> > > +		msg->ack = 0; /* not used */
> > > +		msg->seq = get_cpu_var(fork_counts)++;
> > > +
> > > +		msg->len = CN_FORK_INFO_SIZE;
> > > +		forkmsg = (struct cn_fork_msg *)msg->data;
> > > +		forkmsg->cpu = smp_processor_id();
> > > +		forkmsg->ppid = parent;
> > > +		forkmsg->cpid = child;
> > > +
> > > +		put_cpu_var(fork_counts);
> > > +
> > > +		cn_netlink_send(msg, CN_IDX_FORK, GFP_ATOMIC);
> > 
> > Why is this GFP_ATOMIC?
> 
> In the previous connector, cn_netlink_send(struct cn_msg *msg, u32
> __groups) called alloc_skb(size, GFP_ATOMIC). Now a third parameter is
> used with cn_netlink_send() in order to call alloc_skb(size, gfp_mask)
> with a specific gfp_mask. So, I'm using GFP_ATOMIC as the third argument
> to keep the same behavior.

It was atomic there to allow non process context usage.
Since do_fork() is executed in process context you can use GFP_KERNEL with 
__GFP_NOFAIL  - it will guarantee memory allocation.

> > > +	}
> > > +}
> > > +#else
> > > +static inline void fork_connector(pid_t parent, pid_t child) 
> > > +{
> > > +	return; 
> > > +}
> > > +#endif /* CONFIG_FORK_CONNECTOR */
> > > +#endif /* __KERNEL__ */
> > > +
> > > +#endif /* CN_FORK_H */
> > > Index: linux-2.6.12-rc3-mm3/include/linux/connector.h
> > > ===================================================================
> > > --- linux-2.6.12-rc3-mm3.orig/include/linux/connector.h	2005-05-09 07:45:56.000000000 +0200
> > > +++ linux-2.6.12-rc3-mm3/include/linux/connector.h	2005-05-09 09:50:01.000000000 +0200
> > > @@ -26,6 +26,8 @@
> > >  
> > >  #define CN_IDX_CONNECTOR		0xffffffff
> > >  #define CN_VAL_CONNECTOR		0xffffffff
> > > +#define CN_IDX_FORK			0xfeed  /* fork events */
> > > +#define CN_VAL_FORK			0xbeef
> > >  
> > >  /*
> > >   * Maximum connector's message size.
> > > Index: linux-2.6.12-rc3-mm3/kernel/fork.c
> > > ===================================================================
> > > --- linux-2.6.12-rc3-mm3.orig/kernel/fork.c	2005-05-09 07:45:56.000000000 +0200
> > > +++ linux-2.6.12-rc3-mm3/kernel/fork.c	2005-05-09 08:03:15.000000000 +0200
> > > @@ -41,6 +41,7 @@
> > >  #include <linux/profile.h>
> > >  #include <linux/rmap.h>
> > >  #include <linux/acct.h>
> > > +#include <linux/cn_fork.h>
> > >  
> > >  #include <asm/pgtable.h>
> > >  #include <asm/pgalloc.h>
> > > @@ -63,6 +64,14 @@ DEFINE_PER_CPU(unsigned long, process_co
> > >  
> > >  EXPORT_SYMBOL(tasklist_lock);
> > >  
> > > +#ifdef CONFIG_FORK_CONNECTOR
> > > +/* 
> > > + * fork_counts is used by the fork_connector() inline routine as 
> > > + * the sequence number of the netlink message.
> > > + */
> > > +static DEFINE_PER_CPU(unsigned long, fork_counts); 
> > > +#endif /* CONFIG_FORK_CONNECTOR */
> > > +
> > 
> > The above should go into cn_fork.c
> 
> I don't see why. It's used by fork_connector which is an inline routine
> so, IMHO, 'fork_counts' must be defined here and declared in
> include/linux/cn_fork.h
> 
> > >  int nr_processes(void)
> > >  {
> > >  	int cpu;
> > > @@ -1252,6 +1261,8 @@ long do_fork(unsigned long clone_flags,
> > >  			if (unlikely (current->ptrace & PT_TRACE_VFORK_DONE))
> > >  				ptrace_notify ((PTRACE_EVENT_VFORK_DONE << 8) | SIGTRAP);
> > >  		}
> > > +		
> > > +		fork_connector(current->pid, p->pid);
> > 
> > Are you sure this is what you want? ->pid has a special meaning to the
> > kernel and doesn't necessarily mean the same to user-space, so I think
> > you want ->tgid here. If you look at sys_getpid() and sys_gettid()
> > you'll see what I mean.
> 
> Yes, I think you're right. If I look the code of the BSD process
> accounting they're using the field ->tgid to get the process ID. I fix
> that.
> 
> Thank you very much for your comments,
> Best regards,
> 
> Guillaume


	Evgeniy Polyakov

Only failure makes us experts. -- Theo de Raadt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/