WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 6/8] bio-cgroup: The body of bio-cgroup

Hi,

As you pointed out, cgroup iocontext stuff isn't well designed yet
since the current implementation of dm-iband doesn't need it.
I'd like to leave the iocontext stuff to you I/O scheduler guys
if you want to implement the bio-cgroup infrastructure to handle iocotexts
as the I/O schedulers expect.

> Hi,
> 
> Ryo Tsuruta wrote:
> >  
> > +void init_io_context(struct io_context *ioc)
> > +{
> > +   atomic_set(&ioc->refcount, 1);
> > +   atomic_set(&ioc->nr_tasks, 1);
> > +   spin_lock_init(&ioc->lock);
> > +   ioc->ioprio_changed = 0;
> > +   ioc->ioprio = 0;
> > +   ioc->last_waited = jiffies; /* doesn't matter... */
> > +   ioc->nr_batch_requests = 0; /* because this is 0 */
> > +   ioc->aic = NULL;
> > +   INIT_RADIX_TREE(&ioc->radix_root, GFP_ATOMIC | __GFP_HIGH);
> > +   INIT_HLIST_HEAD(&ioc->cic_list);
> > +   ioc->ioc_data = NULL;
> > +}
> > +
> >  struct io_context *alloc_io_context(gfp_t gfp_flags, int node)
> >  {
> >     struct io_context *ret;
> >  
> >     ret = kmem_cache_alloc_node(iocontext_cachep, gfp_flags, node);
> > -   if (ret) {
> > -           atomic_set(&ret->refcount, 1);
> > -           atomic_set(&ret->nr_tasks, 1);
> > -           spin_lock_init(&ret->lock);
> > -           ret->ioprio_changed = 0;
> > -           ret->ioprio = 0;
> > -           ret->last_waited = jiffies; /* doesn't matter... */
> > -           ret->nr_batch_requests = 0; /* because this is 0 */
> > -           ret->aic = NULL;
> > -           INIT_RADIX_TREE(&ret->radix_root, GFP_ATOMIC | __GFP_HIGH);
> > -           INIT_HLIST_HEAD(&ret->cic_list);
> > -           ret->ioc_data = NULL;
> > -   }
> > +   if (ret)
> > +           init_io_context(ret);
> >  
> >     return ret;
> >  }
> 
> 
> > +
> > +/* Create a new bio-cgroup. */
> > +static struct cgroup_subsys_state *
> > +bio_cgroup_create(struct cgroup_subsys *ss, struct cgroup *cgrp)
> > +{
> > +   struct bio_cgroup *biog;
> > +   struct io_context *ioc;
> > +   int ret;
> > +
> > +   if (!cgrp->parent) {
> > +           biog = &default_bio_cgroup;
> > +           init_io_context(biog->io_context);
> > +           /* Increment the referrence count not to be released ever. */
> > +           atomic_inc(&biog->io_context->refcount);
> > +           idr_init(&bio_cgroup_id);
> > +           return &biog->css;
> > +   }
> > +
> > +   biog = kzalloc(sizeof(*biog), GFP_KERNEL);
> > +   ioc = alloc_io_context(GFP_KERNEL, -1);
> > +   if (!ioc || !biog) {
> > +           ret = -ENOMEM;
> > +           goto out_err;
> > +   }
> > +   biog->io_context = ioc;
> 
> If I understand correctly io_contexts allocated by alloc_io_context() should
> be owned by some tasks. In this case, the newly created io_context has no
> owner though biog->io_context->nr_tasks == 1. With the cfq scheduler this may
> cause some problems especially when cic_free_func() is called because
> cfq_exit_io_context() would never be called. Am I wrong?

I think iocontext allocated for a certain cgroup should be owned by the
cgroup itself, whose code isn't implemented yet. I think you need to
enhance it a bit if the owner is a cgroup.

> > +retry:
> > +   if (!idr_pre_get(&bio_cgroup_id, GFP_KERNEL)) {
> > +           ret = -EAGAIN;
> > +           goto out_err;
> > +   }
> > +   spin_lock_irq(&bio_cgroup_idr_lock);
> > +   ret = idr_get_new_above(&bio_cgroup_id, (void *)biog, 1, &biog->id);
> > +   spin_unlock_irq(&bio_cgroup_idr_lock);
> > +   if (ret == -EAGAIN)
> > +           goto retry;
> > +   else if (ret)
> > +           goto out_err;
> > +
> > +   return &biog->css;
> > +out_err:
> > +   if (biog)
> > +           kfree(biog);
> > +   if (ioc)
> > +           put_io_context(ioc);
> > +   return ERR_PTR(ret);
> > +}
> > +
> > +/* Delete the bio-cgroup. */
> > +static void bio_cgroup_destroy(struct cgroup_subsys *ss, struct cgroup 
> > *cgrp)
> > +{
> > +   struct bio_cgroup *biog = cgroup_bio(cgrp);
> > +
> > +   put_io_context(biog->io_context);
> 
> Here, I suspects what will happen under the following condition:
> biog->io_context->refcount: 1 --> 0
> biog->io_context->nr_tasks: 1 --> 1
> ==> cfq_dtor() will be called but cfq_exit() has not be called yet.
> 
> > +
> > +   spin_lock_irq(&bio_cgroup_idr_lock);
> > +   idr_remove(&bio_cgroup_id, biog->id);
> > +   spin_unlock_irq(&bio_cgroup_idr_lock);
> > +
> > +   kfree(biog);
> > +}
> > +
> 
> > +
> > +/* Determine the iocontext of the bio-cgroup that issued a given bio. */
> > +struct io_context *get_bio_cgroup_iocontext(struct bio *bio)
> > +{
> > +   struct bio_cgroup *biog = NULL;
> > +   struct io_context *ioc;
> > +   int     id = 0;
> > +
> > +   id = get_bio_cgroup_id(bio);
> > +   if (id)
> > +           biog = find_bio_cgroup(id);
> > +   if (!biog)
> > +           biog = &default_bio_cgroup;
> > +   ioc = biog->io_context; /* default io_context for this cgroup */
> > +   atomic_inc(&ioc->refcount);
> > +   return ioc;
> > +}
> > +EXPORT_SYMBOL(get_bio_cgroup_iocontext);
> 
> 
> Thanks,
> Takuya Yoshikawa

Thank you,
Hirokazu Takahashi.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel