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

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.

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