On Mon, Dec 13, 2010 at 11:44:38AM +0800, Tinggong Wang wrote:
> on Mon, 13 Dec 2010 06:48:06AM +0900 Simon Horman (horms@xxxxxxxxxxxx) wrote:
> > [ CCed Hans Schillstrom and Julian Anastasov ]
> >
> > On Sun, Dec 12, 2010 at 07:42:29PM +0800, Tinggong Wang wrote:
> > > Signed-off-by: Tinggong Wang <wangtinggong@xxxxxxxxx>
> > > ---
> > > net/netfilter/ipvs/ip_vs_sync.c | 13 ++++++++-----
> > > 1 files changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
> > > index 7632a17..2b6b0cb 100644
> > > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > > @@ -315,11 +315,6 @@ static void ip_vs_process_message(const char *buffer, const size_t buflen)
> > > char *p;
> > > int i;
> > >
> > > - if (buflen < SYNC_MESG_HEADER_LEN) {
> > > - IP_VS_ERR_RL("sync message header too short\n");
> > > - return;
> > > - }
> > > -
> > > /* Convert size back to host byte order */
> > > m->size = ntohs(m->size);
> > >
> > > @@ -823,6 +818,14 @@ static int sync_thread_backup(void *data)
> > > break;
> > > }
> > >
> > > + /* throw invalid data before local_bh_disable,
> > > + * so performance won't be downgraded by it
> > > + */
> > > + if (len < SYNC_MESG_HEADER_LEN) {
> > > + IP_VS_ERR_RL("sync message header too short\n");
> > > + continue;
> > > + }
> > > +
> > > /* disable bottom half, because it accesses the data
> > > shared by softirq while getting/creating conns */
> > > local_bh_disable();
> > > --
> > > 1.7.2.3
> > >
> >
> > Could you explain the motivation for this change?
>
> in my opinion, before local_bh_disable, should ensure packets are look
> like more resonable.
>
> local_bh_disable will disable all bottom-half processing on local cpu,
> if the multicast group flood of packets containing bad sync message,
> local cpu will be busy doing local_bh_disable and local_bh_enable.
>
> if the backup pc has only one cpu, all other tasks will be pending until
> the flood finished.
Ok, that does sound reasonable to some extent. But realistically
this should only occur if bogus packets are being sent. And in
that case it would be possible for bogus packets to be more carefully
crafted such that we need to enter ip_vs_process_message() anyway.
So I'm not sure if there really is a gain here.
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
