On Fri, Dec 23, 2011 at 10:54:47PM +0800, Changli Gao wrote:
> On Fri, Dec 23, 2011 at 9:42 PM, <pablo@xxxxxxxxxxxxx> wrote:
> > +
> > +static LIST_HEAD(nfnl_acct_list);
>
> You suppose that there won't be many accounting instants. It isn't
> scalable, as we have to iterate the list to find a special item. How
> about a hash table?
The lookup only happens if the rule is loaded.
There's no real gain from using hashing here.
> > +static int
> > +nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > + struct nf_acct *cur, *last;
> > +
> > + if (cb->args[2])
> > + return 0;
> > +
> > + last = (struct nf_acct *)cb->args[1];
> > + if (cb->args[1])
> > + cb->args[1] = 0;
> > +
> > + rcu_read_lock();
> > + list_for_each_entry(cur, &nfnl_acct_list, head) {
>
> You should use the RCU variant here.
I'll fix this, thanks.
> > + if (last && cur != last)
> > + continue;
> > +
> > + if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).pid,
> > + cb->nlh->nlmsg_seq,
> > + NFNL_MSG_ACCT_NEW, cur) < 0) {
> > + cb->args[1] = (unsigned long)cur;
> > + break;
> > + }
> > +
> > + if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) ==
> > + NFNL_MSG_ACCT_GET_CTRZERO) {
> > + atomic64_set(&cur->pkts, 0);
> > + atomic64_set(&cur->bytes, 0);
>
> The get and zero operations should be done in an atomic context,
> otherwise counters added between them will be lost.
Good catch.
I think we can fix this with xchg.
Similar patch should go to nf_conntrack_netlink.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
