go-ping / ping

ICMP Ping library for Go
MIT License
1.32k stars 344 forks source link

CPU overhead when running multiple pingers #139

Closed bestoml closed 1 year ago

bestoml commented 3 years ago

I Have found the issue , below is the different: 113 pings for 300 count. Before Modify : image After : image image

In multi ping situation, each ping has started a receive ICMP goruntine. But receiveICMP will get all the packets received no matter what they are from. For example, sending 100 different addrs, each receiver will receive 100 packet, 99 is wrong id. It will discard in processPacket for id is not the same. But it wastes too much cpu tranfer from chan recv to processPacket. It's about 100*100 received process in one turn , only 100 is right, the other 9900 is wasted. See below log, When receiving a packet id 830,all other ping receiveicmp get the same and discard it, that is the cause : image

So let the ReceiveICMP to be golbal and to be the one. No matter how many pings are running, only one ReceiveICMP goruntine, and then process the packet , get it's id , find the corresponding pinger, and set it's chan. That will solve the problem.

But it may has some defect, for only one receiveicmp, I am not sure that could pingers has both ipv4 and ipv6. But I thought it's not a big problem. Make all the ping the same protocol is not hard to accept.

CHTJonas commented 3 years ago

Thanks for the report! Can I confirm, you've tested it and making the recvICMP() function run at a global scope rather than on a per-pinger basis improves the CPU usage?

bestoml commented 3 years ago

Yes, I change it to global. It works well. For My code is not optimized and patched everywhere. Further more I have changed the mechanism of interval and timeout to each count. Those changes may disturb you. So I didn't copy my codes. You could have a try to upgrade it in current mechanism first.

mem commented 3 years ago

With a very naive implementation, with the current code, pinging ~ 700 IPs from my laptop, load stays below 20%, and most of the time close to 10%.

My code is not logging or doing anything else on the side, just sending out the pings and waiting for them, and it's pinging IPs all around the world (it's picked from the first 1000 hosts in Alexa's top 1 million list).

@bestoml could you please check again to see if this is still an issue?

bestoml commented 3 years ago

With a very naive implementation, with the current code, pinging ~ 700 IPs from my laptop, load stays below 20%, and most of the time close to 10%.

My code is not logging or doing anything else on the side, just sending out the pings and waiting for them, and it's pinging IPs all around the world (it's picked from the first 1000 hosts in Alexa's top 1 million list).

@bestoml could you please check again to see if this is still an issue?

I admire your persistence. I just test again. Maybe is still an issue. Or we can say that It can be improved. The problem you can repeat it easy : Give a mark to this place. Then When doing multiple ping. You will see One Ip will receive all the others responses. if !p.matchID(pkt.ID) { fmt.Printf("%s get other pkt :%d\n",p.Addr(),pkt.ID) return nil } I try a sample to ping 180 iPs as one count .And found each ip will receive 179 useless responses. Because the msg is transfer by chan, It waste the resources. image

It's no need to start : "go p.recvICMP(conn, recv, &wg)" each ip. Just one recv Can get all the response. Then find the pinger by pkt.ID. switch pkt := m.Body.(type) { case *icmp.Echo: //find p pinger, ok :=gPingers[pkt.ID] if ok { if !pinger.isDone() { pinger.chanReceives <- &packet{bytes: bytes, nbytes: n, ttl: ttl} }

            }
        }
bestoml commented 3 years ago

I update my Test sample: https://drive.google.com/file/d/18T-SNYSEasGVHZSHyD5GjoZ6-1iR-Yi5/view?usp=sharing Test in maintest.go func TestProcess

And also my modification version : https://drive.google.com/file/d/1gyJHsZ3CJy8jA5ZCv_dXZydvSuFpuCFE/view?usp=sharing Just run in cmd : go run main.go You can see my way in RecvICMPGlobal. Hope it's helpful for you.