Re: On the road to Xen : LTTng and relay CPU hotplug support

* Mathieu Desnoyers ([email protected]) wrote:
> I would also be tempted to add a callback to tell the client when a CPU is
> added : clients may have to update their private data accordingly.
> 

I should not talk too fast : create_buf_file already does that.

Mathieu

> Mathieu
> 
> * Tom Zanussi ([email protected]) wrote:
> > Mathieu Desnoyers wrote:
> > >Hi,
> > >
> > >In my current work of porting LTTng to the Xen hypervisor, I notice that 
> > >their
> > >project use the Linux CPU hotplug feature extensively in the domain-0 and
> > >domain-Us. This brings me to a preliminary adaptation phase of the LTTng 
> > >kernel
> > >tracer : adding CPU hotplug support.
> > >
> > >The basic idea is that we have (at least) to create a new buffer for every
> > >channel when a CPU is "UP_PREPARE". In order to do that, Relay must be
> > >modified so the buffer creation function is less coupled with the channel
> > >creation and that this function is exported as part of the API.
> > >(I just implemented this modification) We can also destroy the buffer when 
> > >the
> > >CPU goes down (see below).
> > >
> > >Relay cannot, by itself, deal with the CPU hotplug events because it does 
> > >not
> > >have a list of the allocated channels. This is why I choose to put the 
> > >hotplug
> > 
> > Hi,
> > 
> > I think it would be useful to add a channel list to relay and do some of 
> > this there.  Here's a first cut patch that does it - see if it makes 
> > sense for this.  It adds new buffers when cpus come online, but only 
> > flushes them when cpus go offline ,i.e. doesn't destroy the offline buffers.
> > 
> > >
> > >What I could do is to implement (A), keeping in mind that the proper way 
> > >to do
> > >it would be (B), the longer term target therefore being (B). In the 
> > >mindset of
> > >doing incremental changes that "works", I think (A) would be a good start.
> > >
> > >Having your insight on this would be appreciated,
> > >
> > 
> > I think this approach makes sense - since all the buffers will be 
> > destroyed anyway when the channel is destroyed, you could probably avoid 
> > the complications of plan B unless you find you really need to.
> > 
> > Tom
> > 
> > 
> > diff --git a/include/linux/relay.h b/include/linux/relay.h
> > index 24accb4..6969fd2 100644
> > --- a/include/linux/relay.h
> > +++ b/include/linux/relay.h
> > @@ -24,7 +24,7 @@
> >  /*
> >   * Tracks changes to rchan/rchan_buf structs
> >   */
> > -#define RELAYFS_CHANNEL_VERSION        6
> > +#define RELAYFS_CHANNEL_VERSION        7
> > 
> >  /*
> >   * Per-cpu relay channel buffer
> > @@ -64,6 +64,9 @@ struct rchan
> >      void *private_data;        /* for user-defined data */
> >      size_t last_toobig;        /* tried to log event > subbuf size */
> >      struct rchan_buf *buf[NR_CPUS]; /* per-cpu channel buffers */
> > +    struct list_head list;        /* for channel list */
> > +    struct dentry *parent;        /* parent dentry passed to open */
> > +    char base_filename[NAME_MAX];    /* saved base filename */
> >  };
> > 
> >  /*
> > diff --git a/kernel/relay.c b/kernel/relay.c
> > index f04bbdb..5e03f0f 100644
> > --- a/kernel/relay.c
> > +++ b/kernel/relay.c
> > @@ -18,6 +18,11 @@
> >  #include <linux/relay.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/mm.h>
> > +#include <linux/cpu.h>
> > +
> > +/* list of open channels, for cpu hotplug */
> > +static DEFINE_MUTEX(relay_channels_mutex);
> > +static LIST_HEAD(relay_channels);
> > 
> >  /*
> >   * close() vm_op implementation for relay file mapping.
> > @@ -169,6 +174,9 @@ free_buf:
> >  void relay_destroy_channel(struct kref *kref)
> >  {
> >      struct rchan *chan = container_of(kref, struct rchan, kref);
> > +    mutex_lock(&relay_channels_mutex);
> > +    list_del(&chan->list);
> > +    mutex_unlock(&relay_channels_mutex);
> >      kfree(chan);
> >  }
> > 
> > @@ -366,9 +374,9 @@ void relay_reset(struct rchan *chan)
> >      if (!chan)
> >          return;
> > 
> > -    for (i = 0; i < NR_CPUS; i++) {
> > +    for_each_possible_cpu(i) {
> >          if (!chan->buf[i] || chan->buf[i] == prev)
> > -            break;
> > +            continue;
> >          __relay_reset(chan->buf[i], 0);
> >          prev = chan->buf[i];
> >      }
> > @@ -446,6 +454,67 @@ static inline void setup_callbacks(struc
> >      chan->cb = cb;
> >  }
> > 
> > +static int relay_add_hotcpu(int hotcpu)
> > +{
> > +    struct rchan *chan;
> > +    int err = 0, is_global = 0;
> > +    char *tmpname = kmalloc(NAME_MAX + 1, GFP_KERNEL);
> > +    if (!tmpname)
> > +        return -ENOMEM;
> > +
> > +    mutex_lock(&relay_channels_mutex);
> > +    list_for_each_entry(chan, &relay_channels, list) {
> > +        if (chan->buf[hotcpu])
> > +            continue;
> > +        sprintf(tmpname, "%s%d", chan->base_filename, hotcpu);
> > +        chan->buf[hotcpu] = relay_open_buf(chan, tmpname, chan->parent,
> > +                           &is_global);
> > +        if (!chan->buf[hotcpu]) {
> > +            err = -ENOMEM;
> > +            break;
> > +        }
> > +        chan->buf[hotcpu]->cpu = hotcpu;
> > +    }
> > +    mutex_unlock(&relay_channels_mutex);
> > +
> > +    kfree(tmpname);
> > +
> > +    return err;
> > +}
> > +
> > +static void relay_flush_hotcpu(int hotcpu)
> > +{
> > +    struct rchan *chan;
> > +
> > +    mutex_lock(&relay_channels_mutex);
> > +    list_for_each_entry(chan, &relay_channels, list) {
> > +        if (chan->buf[hotcpu])
> > +            continue;
> > +        relay_switch_subbuf(chan->buf[hotcpu], 0);
> > +    }
> > +    mutex_unlock(&relay_channels_mutex);
> > +}
> > +
> > +static int __cpuinit relay_hotcpu_callback(struct notifier_block *nb,
> > +                       unsigned long action,
> > +                       void *hcpu)
> > +{
> > +    unsigned int hotcpu = (unsigned long)hcpu;
> > +
> > +    switch(action) {
> > +    case CPU_UP_PREPARE:
> > +        if (relay_add_hotcpu(hotcpu)) {
> > +            printk(KERN_ERR "relay_add_hotcpu: cpu %d buffer creation 
> > failed\n", hotcpu);
> > +            return NOTIFY_BAD;
> > +        }
> > +        break;
> > +    case CPU_DEAD:
> > +        relay_flush_hotcpu(hotcpu);
> > +        break;
> > +    }
> > +    return NOTIFY_OK;
> > +}
> > +
> >  /**
> >   *    relay_open - create a new relay channel
> >   *    @base_filename: base name of files to create
> > @@ -486,6 +555,8 @@ struct rchan *relay_open(const char *bas
> >      chan->n_subbufs = n_subbufs;
> >      chan->subbuf_size = subbuf_size;
> >      chan->alloc_size = FIX_SIZE(subbuf_size * n_subbufs);
> > +    chan->parent = parent;
> > +    strncpy(chan->base_filename, base_filename, NAME_MAX + 1);
> >      setup_callbacks(chan, cb);
> >      kref_init(&chan->kref);
> > 
> > @@ -503,13 +574,16 @@ struct rchan *relay_open(const char *bas
> >          chan->buf[i]->cpu = i;
> >      }
> > 
> > +    mutex_lock(&relay_channels_mutex);
> > +    list_add(&chan->list, &relay_channels);
> > +    mutex_unlock(&relay_channels_mutex);
> >      kfree(tmpname);
> >      return chan;
> > 
> >  free_bufs:
> > -    for (i = 0; i < NR_CPUS; i++) {
> > +    for_each_possible_cpu(i) {
> >          if (!chan->buf[i])
> > -            break;
> > +            continue;
> >          relay_close_buf(chan->buf[i]);
> >          if (is_global)
> >              break;
> > @@ -622,9 +696,9 @@ void relay_close(struct rchan *chan)
> >      if (!chan)
> >          return;
> > 
> > -    for (i = 0; i < NR_CPUS; i++) {
> > +    for_each_possible_cpu(i) {
> >          if (!chan->buf[i] || chan->buf[i] == prev)
> > -            break;
> > +            continue;
> >          relay_close_buf(chan->buf[i]);
> >          prev = chan->buf[i];
> >      }
> > @@ -652,9 +726,9 @@ void relay_flush(struct rchan *chan)
> >      if (!chan)
> >          return;
> > 
> > -    for (i = 0; i < NR_CPUS; i++) {
> > +    for_each_possible_cpu(i) {
> >          if (!chan->buf[i] || chan->buf[i] == prev)
> > -            break;
> > +            continue;
> >          relay_switch_subbuf(chan->buf[i], 0);
> >          prev = chan->buf[i];
> >      }
> > @@ -1021,3 +1095,12 @@ struct file_operations relay_file_operat
> >      .sendfile       = relay_file_sendfile,
> >  };
> >  EXPORT_SYMBOL_GPL(relay_file_operations);
> > +
> > +static __init int relay_init(void)
> > +{
> > +    hotcpu_notifier(relay_hotcpu_callback, 0);
> > +
> > +    return 0;
> > +}
> > +
> > +module_init(relay_init);
> > 
> OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
> Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68 
> _______________________________________________
> Ltt-dev mailing list
> [email protected]
> http://listserv.shafik.org/mailman/listinfo/ltt-dev
> 
OpenPGP public key:              http://krystal.dyndns.org:8080/key/compudj.gpg
Key fingerprint:     8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68