On Tue, 2007-08-14 at 01:01 -0700, Dor Laor wrote:
> Why not use the standard naming for guest/host or front/backend?
I was trying to stress that this isn't virtualization specific. For
instance, you could use an IOQ on something like an AMP or RDMA based
system too. However, that being said I am not married to the
generalized terms. I could easily change the locality code to be
"guest/host" and be just as happy if it eliminates confusion.
> I didn’t see usage for cookie in downstream patches. Is it obslete?
It's use is optional, but the IOQNET driver does take advantage of it.
It stores things like the real skb pointer.
>
> >+ u64 ptr;
> >+ u64 offset;
>
> The ptr & offset can be united.
Ya, you are probably right. In earlier designs I didn't have the cookie
field so I wanted ptr to remain fixed at the base of the allocation.
That is probably not as important any more.
> What's alen?
"actual length". For instance, look at how IOQNET manages the the
(guest-side) rx queue. It populates the ring with a bunch of MTU sized
skbs and sets len = MTU, alen = 0. The host side then comes and adjusts
the alen to match what the actual packet length is.
>
> >+ u8 valid;
> >+ u8 sown; /* South owned = 1, North owned = 0 */
>
> What about no-one owns it?
There is *always* an owner ;)
> if you plan of using the valid field you might need
> to handle complex races.
If we were only using the valid/sown flags that would probably be true.
But the ioq_ring_indexes are also used and I think they eliminate the
races.
Typically an application will move to either the head of tail of an
index, and then process until the point that the valid/sown flags
indicate we are done. I think you will find this is race free, but
certainly let me know if you think otherwise.
>
> >+};
> >+
> >+#define IOQ_RING_MAGIC 0x47fa2fe4
> >+#define IOQ_RING_VER 1
> >+
> >+struct ioq_ring_idx {
> >+ u32 head; /* 0 based index to head of ptr array
> >*/
> >+ u32 tail; /* 0 based index to tail of ptr array
> >*/
> >+ u8 full;
>
> full = head==tail ???
Sometimes ;)
I might have screwed this up, but as far as I know the logic is sound.
The way I figured it, there are two conditions where we can have
head==tail. When the ring is empty, or when its full. The full flag
allows them to be differentiated.
> Why have a notifier in a separate structure?
The notifier structure is something allocated and provided by the user
of the ioq. For an example, see the IOQNET driver.
> >+
> >+int ioq_iter_push(struct ioq_iterator *iter, int flags)
> >+{
> >+ struct ioq_ring_head *head_desc = iter->ioq->head_desc;
> >+ struct ioq_ring_idx *idx = iter->idx;
> >+ int ret = -ENOSPC;
> >+
> >+ /*
> >+ * Its only valid to push if we are currently pointed at the head
> >+ */
> >+ if (iter->pos != idx->head)
> >+ return -EINVAL;
> >+
> >+ if (ioq_ring_count(idx, head_desc->count) < head_desc->count) {
> >+ idx->head++;
> >+ idx->head %= head_desc->count;
> >+
> >+ if (idx->head == idx->tail)
> >+ idx->full = 1;
> >+
> >+ ret = ioq_iter_seek(iter, ioq_seek_next, 0, flags);
>
> Instead of the above code I would call to iter_seek and do everything inside
> then return a proper error code if needed.
Im not sure I fully understand your point. But if I do get what you are
saying that wouldn't work right for the design. iter_push and iter_pop
are special forms of iter_seek(seek_next) in that they advance the index
AND the position. iter_seek only advances the position.
> EXPORT_SYMBOL(ioq_iter_pop);
> >+
> >+int ioq_iter(struct ioq *ioq, struct ioq_iterator *iter,
> >+ enum ioq_idx_type type, int flags)
>
> ioq_iter_init ?
yeah, thats a good way to think of it. I think I originally had a more
descriptive name and then got on a short "ioq_xxxx" kick ;)
-Greg
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
