Re: [PATCH 5/8] lguest: trivial guest network driver
On Thu, 2007-02-15 at 00:57 +1100, Herbert Xu wrote: > On Wed, Feb 14, 2007 at 03:47:55PM +1100, Rusty Russell wrote: > > > > It's also used to generate dma structs for outgoing packets. In that > > case, skb_headlen() == 0: > > I see, in that case you're guaranteed to have no fragments. > Still it feels a bit weird to have a length field that only > applies to the header part. Indeed. In future if I want to create fragmented skbs for receive (large MTU support), I would actually set skb->len and skb->datalen and then could avoid the extra argument. Cheers, Rusty. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Wed, Feb 14, 2007 at 03:47:55PM +1100, Rusty Russell wrote:
>
> It's also used to generate dma structs for outgoing packets. In that
> case, skb_headlen() == 0:
I see, in that case you're guaranteed to have no fragments.
Still it feels a bit weird to have a length field that only
applies to the header part.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Wed, Feb 14, 2007 at 03:47:55PM +1100, Rusty Russell wrote:
It's also used to generate dma structs for outgoing packets. In that
case, skb_headlen() == 0:
I see, in that case you're guaranteed to have no fragments.
Still it feels a bit weird to have a length field that only
applies to the header part.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Thu, 2007-02-15 at 00:57 +1100, Herbert Xu wrote: On Wed, Feb 14, 2007 at 03:47:55PM +1100, Rusty Russell wrote: It's also used to generate dma structs for outgoing packets. In that case, skb_headlen() == 0: I see, in that case you're guaranteed to have no fragments. Still it feels a bit weird to have a length field that only applies to the header part. Indeed. In future if I want to create fragmented skbs for receive (large MTU support), I would actually set skb-len and skb-datalen and then could avoid the extra argument. Cheers, Rusty. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Wed, 2007-02-14 at 01:06 +1100, Herbert Xu wrote:
> On Tue, Feb 13, 2007 at 01:15:18PM +1100, Rusty Russell wrote:
> >
> > Good spotting! This function needs to be passed skb_headlen(skb),
> > rather than skb->len. Patch is below (I renamed the parameter as well,
> > for clarity).
>
> How about just dropping that parameter and using skb_headlen(skb)
> directly?
It's also used to generate dma structs for outgoing packets. In that
case, skb_headlen() == 0:
static struct sk_buff *lguestnet_alloc_skb(struct net_device *dev, int gfpflags)
{
struct sk_buff *skb;
skb = alloc_skb(16 + ETH_HLEN + DATA_SIZE, gfpflags);
if (!skb)
return NULL;
skb->dev = dev;
skb_reserve(skb, 16);
return skb;
}
/* Find a new skb to put in this slot in shared mem. */
static int fill_slot(struct net_device *dev, unsigned int slot)
{
struct lguestnet_info *info = dev->priv;
/* Try to create and register a new one. */
info->skb[slot] = lguestnet_alloc_skb(dev, GFP_ATOMIC);
if (!info->skb[slot]) {
printk("%s: could not fill slot %i\n", dev->name, slot);
return -ENOMEM;
}
skb_to_dma(info->skb[slot], ETH_HLEN + DATA_SIZE, >dma[slot]);
Cheers,
Rusty.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Tue, Feb 13, 2007 at 01:15:18PM +1100, Rusty Russell wrote:
>
> Good spotting! This function needs to be passed skb_headlen(skb),
> rather than skb->len. Patch is below (I renamed the parameter as well,
> for clarity).
How about just dropping that parameter and using skb_headlen(skb)
directly?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Tue, Feb 13, 2007 at 01:15:18PM +1100, Rusty Russell wrote:
Good spotting! This function needs to be passed skb_headlen(skb),
rather than skb-len. Patch is below (I renamed the parameter as well,
for clarity).
How about just dropping that parameter and using skb_headlen(skb)
directly?
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Wed, 2007-02-14 at 01:06 +1100, Herbert Xu wrote:
On Tue, Feb 13, 2007 at 01:15:18PM +1100, Rusty Russell wrote:
Good spotting! This function needs to be passed skb_headlen(skb),
rather than skb-len. Patch is below (I renamed the parameter as well,
for clarity).
How about just dropping that parameter and using skb_headlen(skb)
directly?
It's also used to generate dma structs for outgoing packets. In that
case, skb_headlen() == 0:
static struct sk_buff *lguestnet_alloc_skb(struct net_device *dev, int gfpflags)
{
struct sk_buff *skb;
skb = alloc_skb(16 + ETH_HLEN + DATA_SIZE, gfpflags);
if (!skb)
return NULL;
skb-dev = dev;
skb_reserve(skb, 16);
return skb;
}
/* Find a new skb to put in this slot in shared mem. */
static int fill_slot(struct net_device *dev, unsigned int slot)
{
struct lguestnet_info *info = dev-priv;
/* Try to create and register a new one. */
info-skb[slot] = lguestnet_alloc_skb(dev, GFP_ATOMIC);
if (!info-skb[slot]) {
printk(%s: could not fill slot %i\n, dev-name, slot);
return -ENOMEM;
}
skb_to_dma(info-skb[slot], ETH_HLEN + DATA_SIZE, info-dma[slot]);
Cheers,
Rusty.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Tue, 2007-02-13 at 02:55 +1100, Herbert Xu wrote:
> On Mon, Feb 12, 2007 at 02:52:01PM +1100, Rusty Russell wrote:
> >
> > +static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
> > + struct lguest_dma *dma)
> > +{
> > + unsigned int i, seg;
> > +
> > + for (i = seg = 0; i < len; seg++, i += rest_of_page(skb->data + i)) {
>
> The length field from the skb covers the fragmented parts as well.
> So you don't want to use it as the boundary for the loop over the
> skb header data. As it is, if the skb has fragments, the first
> loop will try to read them off the header.
Good spotting! This function needs to be passed skb_headlen(skb),
rather than skb->len. Patch is below (I renamed the parameter as well,
for clarity).
It's fascinating why this "worked". Firstly, for inter-guest
communication, I am not calculating checksums, nor checking them.
Secondly, for guest->host, I turn off hw checksumming so the kernel
disables SG and this code is never executed. Thirdly, for virtbench's
inter-guest sendfile test does not check the data received is actually
correct. Finally, while we end up producing a packet which is larger
than skb->len of 1514, the DMA receive buffer for the other guest is
only 1514 bytes, so the result of the transfer is 1514 (==skb->len), so
no error reported by the driver.
==
lguest network driver fix: handle scatter-gather packets correctly
Thanks to Herbert Xu for noticing the bug: "len" here is skb_headlen(),
not skb->len. Renamed the function to clarify, too.
Signed-off-by: Rusty Russell <[EMAIL PROTECTED]>
diff -r ed6484d145a4 drivers/net/lguest_net.c
--- a/drivers/net/lguest_net.c Tue Feb 13 11:30:39 2007 +1100
+++ b/drivers/net/lguest_net.c Tue Feb 13 12:07:15 2007 +1100
@@ -59,14 +59,14 @@ static unsigned long peer_addr(struct lg
return info->peer_phys + 4 * peernum;
}
-static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
+static void skb_to_dma(const struct sk_buff *skb, unsigned int headlen,
struct lguest_dma *dma)
{
unsigned int i, seg;
- for (i = seg = 0; i < len; seg++, i += rest_of_page(skb->data + i)) {
+ for (i = seg = 0; i < headlen; seg++, i += rest_of_page(skb->data+i)) {
dma->addr[seg] = virt_to_phys(skb->data + i);
- dma->len[seg] = min((unsigned)(len - i),
+ dma->len[seg] = min((unsigned)(headlen - i),
rest_of_page(skb->data + i));
}
for (i = 0; i < skb_shinfo(skb)->nr_frags; i++, seg++) {
@@ -90,7 +90,7 @@ static void transfer_packet(struct net_d
struct lguestnet_info *info = dev->priv;
struct lguest_dma dma;
- skb_to_dma(skb, skb->len, );
+ skb_to_dma(skb, skb_headlen(skb), );
pr_debug("xfer length %04x (%u)\n", htons(skb->len), skb->len);
hcall(LHCALL_SEND_DMA, peer_addr(info,peernum), __pa(),0);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Mon, Feb 12, 2007 at 02:52:01PM +1100, Rusty Russell wrote:
>
> +static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
> +struct lguest_dma *dma)
> +{
> + unsigned int i, seg;
> +
> + for (i = seg = 0; i < len; seg++, i += rest_of_page(skb->data + i)) {
> + dma->addr[seg] = virt_to_phys(skb->data + i);
> + dma->len[seg] = min((unsigned)(len - i),
> + rest_of_page(skb->data + i));
> + }
> + for (i = 0; i < skb_shinfo(skb)->nr_frags; i++, seg++) {
> + const skb_frag_t *f = _shinfo(skb)->frags[i];
> + /* Should not happen with MTU less than 64k - 2 * PAGE_SIZE. */
> + if (seg == LGUEST_MAX_DMA_SECTIONS) {
> + printk("Woah dude! Megapacket!\n");
> + break;
> + }
> + dma->addr[seg] = page_to_phys(f->page) + f->page_offset;
> + dma->len[seg] = f->size;
> + }
The length field from the skb covers the fragmented parts as well.
So you don't want to use it as the boundary for the loop over the
skb header data. As it is, if the skb has fragments, the first
loop will try to read them off the header.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Mon, Feb 12, 2007 at 02:52:01PM +1100, Rusty Russell wrote:
+static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
+struct lguest_dma *dma)
+{
+ unsigned int i, seg;
+
+ for (i = seg = 0; i len; seg++, i += rest_of_page(skb-data + i)) {
+ dma-addr[seg] = virt_to_phys(skb-data + i);
+ dma-len[seg] = min((unsigned)(len - i),
+ rest_of_page(skb-data + i));
+ }
+ for (i = 0; i skb_shinfo(skb)-nr_frags; i++, seg++) {
+ const skb_frag_t *f = skb_shinfo(skb)-frags[i];
+ /* Should not happen with MTU less than 64k - 2 * PAGE_SIZE. */
+ if (seg == LGUEST_MAX_DMA_SECTIONS) {
+ printk(Woah dude! Megapacket!\n);
+ break;
+ }
+ dma-addr[seg] = page_to_phys(f-page) + f-page_offset;
+ dma-len[seg] = f-size;
+ }
The length field from the skb covers the fragmented parts as well.
So you don't want to use it as the boundary for the loop over the
skb header data. As it is, if the skb has fragments, the first
loop will try to read them off the header.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/8] lguest: trivial guest network driver
On Tue, 2007-02-13 at 02:55 +1100, Herbert Xu wrote:
On Mon, Feb 12, 2007 at 02:52:01PM +1100, Rusty Russell wrote:
+static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
+ struct lguest_dma *dma)
+{
+ unsigned int i, seg;
+
+ for (i = seg = 0; i len; seg++, i += rest_of_page(skb-data + i)) {
The length field from the skb covers the fragmented parts as well.
So you don't want to use it as the boundary for the loop over the
skb header data. As it is, if the skb has fragments, the first
loop will try to read them off the header.
Good spotting! This function needs to be passed skb_headlen(skb),
rather than skb-len. Patch is below (I renamed the parameter as well,
for clarity).
It's fascinating why this worked. Firstly, for inter-guest
communication, I am not calculating checksums, nor checking them.
Secondly, for guest-host, I turn off hw checksumming so the kernel
disables SG and this code is never executed. Thirdly, for virtbench's
inter-guest sendfile test does not check the data received is actually
correct. Finally, while we end up producing a packet which is larger
than skb-len of 1514, the DMA receive buffer for the other guest is
only 1514 bytes, so the result of the transfer is 1514 (==skb-len), so
no error reported by the driver.
==
lguest network driver fix: handle scatter-gather packets correctly
Thanks to Herbert Xu for noticing the bug: len here is skb_headlen(),
not skb-len. Renamed the function to clarify, too.
Signed-off-by: Rusty Russell [EMAIL PROTECTED]
diff -r ed6484d145a4 drivers/net/lguest_net.c
--- a/drivers/net/lguest_net.c Tue Feb 13 11:30:39 2007 +1100
+++ b/drivers/net/lguest_net.c Tue Feb 13 12:07:15 2007 +1100
@@ -59,14 +59,14 @@ static unsigned long peer_addr(struct lg
return info-peer_phys + 4 * peernum;
}
-static void skb_to_dma(const struct sk_buff *skb, unsigned int len,
+static void skb_to_dma(const struct sk_buff *skb, unsigned int headlen,
struct lguest_dma *dma)
{
unsigned int i, seg;
- for (i = seg = 0; i len; seg++, i += rest_of_page(skb-data + i)) {
+ for (i = seg = 0; i headlen; seg++, i += rest_of_page(skb-data+i)) {
dma-addr[seg] = virt_to_phys(skb-data + i);
- dma-len[seg] = min((unsigned)(len - i),
+ dma-len[seg] = min((unsigned)(headlen - i),
rest_of_page(skb-data + i));
}
for (i = 0; i skb_shinfo(skb)-nr_frags; i++, seg++) {
@@ -90,7 +90,7 @@ static void transfer_packet(struct net_d
struct lguestnet_info *info = dev-priv;
struct lguest_dma dma;
- skb_to_dma(skb, skb-len, dma);
+ skb_to_dma(skb, skb_headlen(skb), dma);
pr_debug(xfer length %04x (%u)\n, htons(skb-len), skb-len);
hcall(LHCALL_SEND_DMA, peer_addr(info,peernum), __pa(dma),0);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
