[ovs-dev] [PATCH 2/2] datapath: Move over-MTU checking into vport_send().

Jesse Gross jesse at nicira.com
Tue Jul 13 21:11:00 UTC 2010


On Tue, Jul 13, 2010 at 11:43 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Jul 12, 2010 at 02:31:17PM -0700, Jesse Gross wrote:
> > @@ -1276,6 +1277,17 @@ vport_receive(struct vport *vport, struct sk_buff
> *skb)
> >       dp_process_received_packet(dp_port, skb);
> >  }
> >
> > +static inline unsigned
> > +packet_length(const struct sk_buff *skb)
>
> Kernel style usually keeps the return type and the arguments on a single
> line.  I guess we're not doing that in vport.c.  Perhaps we should.
>

There are a bunch of places where we do this.  When I get a chance, I'll
just make a pass through all of our kernel code and make them consistent.


>
> >  /**
> >   *   vport_send - send a packet on a device
> >   *
> > @@ -1289,24 +1301,26 @@ int
> >  vport_send(struct vport *vport, struct sk_buff *skb)
> >  {
> >       int *loop_count;
> > +     int mtu;
> >       int sent;
> >
> >       loop_count = &per_cpu_ptr(vport_loop_counter,
> get_cpu())->count[!!in_interrupt()];
> >       (*loop_count)++;
> >
> > -     if (likely((*loop_count) <= VPORT_MAX_LOOPS)) {
> > -             sent = vport->ops->send(vport, skb);
> > -     } else {
> > +     if (unlikely((*loop_count) > VPORT_MAX_LOOPS)) {
>
> I think that the extra () around *loop_count makes this harder to read.
> Mind dropping them?
>

Yeah, I probably did it because I just finished typing it for the line
above.


>
> >               printk(KERN_WARNING "%s: dropping packet that has looped
> more than %d times\n",
> >                      dp_name(vport_get_dp_port(vport)->dp),
> VPORT_MAX_LOOPS);
> > +             goto error;
> > +     }
>
> Shouldn't we rate-limit that message?
>

Yeah, I added that.


>
> (I should have pointed both of those out for patch 1/2.)
>
> >
> > -             sent = 0;
> > -             kfree_skb(skb);
> > -             vport_record_error(vport, VPORT_E_TX_DROPPED);
> > +     mtu = vport_get_mtu(vport);
> > +     if (unlikely(packet_length(skb) > mtu && !skb_is_gso(skb))) {
> > +             printk(KERN_WARNING "%s: dropped over-mtu packet: %d >
> %d\n",
> > +                    dp_name(vport_get_dp_port(vport)->dp),
> packet_length(skb), mtu);
> > +             goto error;
> >       }
>
> It seems like we should rate-limit this one too.  I wonder why we don't.
>

Added it here too.

I pushed these two patches.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100713/d606af7d/attachment-0003.html>


More information about the dev mailing list