Re: [Xen-devel] [PATCH V9 08/12] remus: implement the API to buffer/release packages

[Shriram Rajagopalan <rshriram@xxxxxxxxx>]

On Wed, Apr 23, 2014 at 11:10 AM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:

Yang Hongyang writes ("[PATCH V9 08/12] remus: implement the API to buffer/release packages"):

> This patch implements two APIs:
> 1. netbuf_start_new_epoch()
> Â ÂIt marks a new epoch. The packages before this epoch will
> Â Âbe flushed, and the packages after this epoch will be buffered.
> Â ÂIt will be called after the guest is suspended.
> 2. netbuf_release_prev_epoch()
> Â ÂIt flushes the buffered packages to client, and it will be
> Â Âcalled when a checkpoint finishes.

Thanks.

(BTW "packages" should be "packets" throughout.)

> diff --git a/tools/libxl/libxl_netbuffer.c b/tools/libxl/libxl_netbuffer.c
> index a5f2b9a..4b4bc9d 100644
> --- a/tools/libxl/libxl_netbuffer.c
> +++ b/tools/libxl/libxl_netbuffer.c
> @@ -416,9 +416,66 @@ static int nic_teardown(libxl__remus_device *remus_dev,
> Â Â Âreturn REMUS_INPROGRESS;
> Â}
>
> +/* The buffer_op's value, not the value passed to kernel */
> +enum {
> + Â Âtc_buffer_start,
> + Â Âtc_buffer_release
> +};

The comment refers to a "buffer_op" variable which is not nearby. ÂI
suggest replacing the comment with
  /* Internal value for libxl, not the value passed to kernel */

Are these names "tc_" not possibly clashing with some future kernel
things ?

These are local enums. They have nothing to do with the sch_plug

qdisc in the kernel. That said, I agree that they could be renamed to

something more local:

Ânetbuf_begin_next_epoch /* start buffering pkts for next epoch */

Ânetbuf_commit_prev_epoch Â/* release buffered pkts from prev epoch, after

                          Â* receiving commit ack from backup

                          Â*/

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel